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

Favor project-specific build scripts over top-level script #29918

Merged
merged 13 commits into from
Feb 16, 2021

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Feb 5, 2021

Unfortunately, it doesn't look like we can totally move the build.sh/build.cmd scripts out of the top-level directory and into eng since various parts of Arcade rely on build scripts being in the top-level. But this PR:

  • Updates projects without build scripts to include them
  • Updates our docs to highlight using projects
  • Adds a warning text to the top-level build scripts if it is being executed in local mode

Addresses #27631

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 5, 2021
@captainsafia captainsafia force-pushed the safia/fix-build-scripts branch 3 times, most recently from a1fb62a to 011ac76 Compare February 5, 2021 19:58
@captainsafia captainsafia changed the title Remove top-level build scripts in favor of project specific scripts Favor project-specific build scripts over top-level script Feb 10, 2021
@captainsafia captainsafia marked this pull request as ready for review February 11, 2021 02:55
@captainsafia captainsafia requested a review from a team February 11, 2021 02:55
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Need build.* scripts in

  • src/Configuration.KeyPerFile/
  • src./DefaultBuilder/
  • src/Features/
  • src/FileProviders/
  • src/Html/
  • src/HttpClientFactory/
  • src/Installers/Debian
  • src/Installers/Rpm
  • src/Logging.AzureAppServices/
  • src/ObjectPool/
  • src/Shared/
  • src/Testing/
  • src/WebEncoders/

build.ps1 Outdated Show resolved Hide resolved
@HaoK
Copy link
Member

HaoK commented Feb 11, 2021

Should we have a (non helix) test that just runs all of these build scripts to ensure they work in some way (exit code 0?)

@dougbu
Copy link
Member

dougbu commented Feb 12, 2021

Should we have a (non helix) test that just runs all of these build scripts to ensure they work in some way (exit code 0?)

We have a pipeline executing .azure\pipelines\devBuild.yml but removed or disabled it because nobody was paying attention. If you have suggestions to extend that YAML and make the pipeline useful to the team, fine. But, that said, the scripts are mostly 💯% identical and I can't see how they would fail without something almost intentionally messed up e.g. lots of weird references to bin/ or obj/ folders without project references.

@HaoK
Copy link
Member

HaoK commented Feb 12, 2021

We have a pipeline executing .azure\pipelines\devBuild.yml

Right because its a separate pipeline, while if we had a normal azdo test that ran every one of these scripts as part of ci builds, it wouldn't be ignorable and would guarantee these scripts always work (and are reasonably not flaky)

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Getting very close though CI disagrees

.azure/pipelines/ci.yml Outdated Show resolved Hide resolved
src/Html/Abstractions/build.cmd Outdated Show resolved Hide resolved
src/Html/Abstractions/build.cmd Outdated Show resolved Hide resolved
src/Html/Abstractions/build.sh Outdated Show resolved Hide resolved
src/Installers/Windows/build.ps1 Show resolved Hide resolved
eng/build.ps1 Show resolved Hide resolved
eng/build.ps1 Show resolved Hide resolved
@dougbu
Copy link
Member

dougbu commented Feb 12, 2021

had a normal azdo test that ran every one of these scripts as part of ci builds

Main concerns there are the effort required to clean the repo after each script runs (because project references mean they're highly intertwined) and the overall time it would take. Using Helix might be easier though it may be difficult to get right initially e.g. does the Helix SDK support this❔

@HaoK
Copy link
Member

HaoK commented Feb 12, 2021

Using Helix might be easier though it may be difficult to get right initially e.g. does the Helix SDK support this❔

I guess helix would work pretty well as a new test that basically verified contributor workflow, could just exercise the getting started instructions / clone the repro, install preqs, do a build, and also do a clean and try each individual sub build script. Would be easier to just do that on the existing build machines as they are already setup, but there's decent value in ensuring that our repo always works from a clean state

@@ -224,6 +224,8 @@ code .

When developing in VS Code, you'll need to use the `build.cmd` or `build.sh` scripts in order to build the project. You can learn more about the command line options available, check out [the section below](using-dotnet-on-command-line-in-this-repo).

After navigating to the parent directory of the desired project, you can run the build script for that individual project.
Copy link
Member

Choose a reason for hiding this comment

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

Does "parent directory" here mean something like "closest ancestor directory that contains a build.cmd/build.sh file"? Many of the .csproj files don't have a build script in their immediate parent directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct -- I've added some more text/samples here to explain the relationship between the build scripts and the project modified.

@captainsafia captainsafia merged commit 0801cea into main Feb 16, 2021
@captainsafia captainsafia deleted the safia/fix-build-scripts branch February 16, 2021 17:26
dougbu pushed a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021
…pnetcore#29918)

* Remove top-level build scripts in favor of project specific scripts
* Add more build scripts and move top-level to eng
* Fix path in Powershell script
* Update BuildDirectory for CI jobs
* Fix paths in restore scripts and jobs
* Address feedback from peer review
* Fix references to Html.Abstractions
* Update baseline files with new project
* Fix a few more broken links
* !fixup! Correct Build.props and regen project references
* Update docs and fix public api modification check

Co-authored-by: Doug Bunting <[email protected]>

Commit migrated from dotnet/aspnetcore@0801cea403b2
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.

6 participants