Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A quick collection of quick fixes #20993

Merged
merged 6 commits into from
Apr 26, 2020
Merged

A quick collection of quick fixes #20993

merged 6 commits into from
Apr 26, 2020

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Apr 19, 2020

  • simplify devBuilds.yml
  • clean up SiteExtensions/build.cmd
  • move SetupNugetSources script invocations above parameters.beforeBuild
  • ensure $(BuildNative) is always set correctly
  • support -all together with -projects
  • consistently use --build-*

(Mostly found while working on #20748. Some parts will make it easier to use dotnet more if we continue down that road.)

@dougbu
Copy link
Member Author

dougbu commented Apr 19, 2020

Additional verification builds

@dougbu dougbu force-pushed the dougbu/quick.fixes branch 2 times, most recently from 3c4a685 to 8ee921f Compare April 19, 2020 07:31
@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Apr 19, 2020
@dougbu dougbu force-pushed the dougbu/quick.fixes branch 3 times, most recently from eaea3ec to b0f4606 Compare April 21, 2020 01:50
@dougbu dougbu marked this pull request as ready for review April 21, 2020 05:37
@dougbu dougbu requested a review from a team April 21, 2020 05:37
@dougbu
Copy link
Member Author

dougbu commented Apr 21, 2020

Oops, missing a commit and I forgot to rerun some validation builds. Back to draft…

@dougbu dougbu marked this pull request as draft April 21, 2020 05:41
@dougbu dougbu force-pushed the dougbu/quick.fixes branch from b0f4606 to 1102190 Compare April 21, 2020 22:24
@@ -31,12 +31,12 @@ stages:
steps:
- script: git submodule init
- script: git submodule update --recursive
- script: cd ./src/Components
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkArtakMSFT you may want to look at this file in particular

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. I initially have totally forgotten that this is something I had created. Thanks @pranavkm for reminding this 👍
Anyway, the change looks reasonable. We should discuss in our next meeting whether this is still useful. We tend to look at the official CI views rather than this any more, so maybe it's time to completely stop this.

@@ -1,4 +1,6 @@
<Project>
<Import Project="eng\Common.props" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, $(BuildNative) was only available in the projects that needed it when property was global ☹️

@@ -1,26 +1,40 @@
@ECHO OFF
SET RepoRoot=%~dp0..\..

ECHO Building Microsoft.AspNetCore.Runtime.SiteExtension
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JunTaoLuo this one's for you 😈

@dougbu dougbu marked this pull request as ready for review April 22, 2020 05:25
@dougbu
Copy link
Member Author

dougbu commented Apr 22, 2020

Marking this as ready to review though validation builds (all but devBuilds.yml one) are failing due to what appear to be general infrastructure issues

@dougbu dougbu requested review from mkArtakMSFT and JunTaoLuo April 22, 2020 05:26
dougbu added 6 commits April 23, 2020 15:01
- remove an excess build step
- quote all rooted paths
- check `%ERRORLEVEL%` after every `CALL`
- nits:
  - add a few more `ECHO` commands
  - wrap long lines
…ers.beforeBuild`

- ensure NuGet.config is ready for all internal builds
- remove now-duplicate `SetupNugetSources` invocations wherever default-build.yml is used
- fix problems using `-all` or `/p:BuildAllProjects=true` without `-buildNative`
  - ensure `$(BuildNative)` is `false` where it's not supported
- move some duplicated settings into eng/Common.props and `<Import />` the new file
- remove now-duplicated parts of conditions using `$(BuildNative)`
- remove need to specify `/p:BuildAllProjects=true`
- nit: simplify some Boolean logic
- avoid `/p:Build*`  on the command line (except with eng/scripts/ci-source-build.sh)
- nits:
  - remove now-useless `-buildNative` with `-all`
  - expand and correct a couple of related comments and messages
@dougbu dougbu force-pushed the dougbu/quick.fixes branch from 1102190 to bc8ab4e Compare April 23, 2020 22:04
@dougbu
Copy link
Member Author

dougbu commented Apr 23, 2020

🆙📅 to pick up latest from 'master'. @mkArtakMSFT and @JunTaoLuo please weigh in.

Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dougbu dougbu merged commit 54722a5 into master Apr 26, 2020
@dougbu dougbu deleted the dougbu/quick.fixes branch April 26, 2020 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants