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

Support DotNetBuildPass property in Arcade's Build.proj #14660

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ViktorHofer
Copy link
Member

  1. Support DotNetBuildPass property Only build ProjectToBuild items with DotNetBuildPass metadata matching the passed-in value of DotNetBuildPass Don't glob for solution files in that build mode. Don't invoke the Execute target when nothing should be built.
  2. Add SkipProjectToBuildError property to not error out when ProjectToBuild items are empty. This makes clean-up in source-build's outer build infrastructure possible (i.e. removal of Noop.proj)
  3. Simplify Publish.proj Exec conditions in Build.proj
  4. Add the DotNetBuildPass metadata to the asset manifest name when set. Also respect TargetArchitecture and TargetOS in the manifest name over Platform and OS.
  5. Allow repos to invoke Arcade's Build.proj with the -restore -sign -publish actions only. For that to work, skip the ProjectToBuild empty error check when none of the build actions are passed in.

To double check:

@ViktorHofer ViktorHofer force-pushed the SupportJoinPoint branch 2 times, most recently from 1b58721 to 4b47eee Compare March 27, 2024 12:33
1. Support DotNetBuildPass property
   Only build ProjectToBuild items with DotNetBuildPass
   metadata matching the passed-in value of DotNetBuildPass
   Don't glob for solution files in that build mode.
   Don't invoke the Execute target when nothing should be built.
2. Add SkipProjectToBuildError property to not
   error out when ProjectToBuild items are empty.
   This makes clean-up in source-build's outer build
   infrastructure possible (i.e. removal of Noop.proj)
3. Simplify Publish.proj Exec conditions in Build.proj
4. Add the DotNetBuildPass metadata to the asset manifest
   name when set. Also respect TargetArchitecture and
   TargetOS in the manifest name over Platform and OS.
5. Allow repos to invoke Arcade's Build.proj with the
   `-restore -sign -publish` actions only. For that to
   work, skip the ProjectToBuild empty error check when
   none of the build actions are passed in.
<_VmrBuild Condition="'$(DotNetBuildFromSourceFlavor)' == 'Product' or '$(DotNetBuildOrchestrator)' == 'true'">true</_VmrBuild>

<_OuterRepoBuild Condition="'$(ArcadeInnerBuildFromSource)' != 'true' and '$(DotNetBuildPhase)' != 'InnerRepo'">true</_OuterRepoBuild>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need the legacy switches here. Arcade must pass the legacy switches to support repos that don't use the new switches within their repo code, but Arcade shouldn't need to use switches any longer. You can just have comparisons against DotNetBuildPhase (Repo, InnerRepo and Orchestrator)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that's true. A repository that is still on Arcade 8 wouldn't have DotNetBuildPhase set to anything in the outer repo non-VMR build.

That's because repos on the old Arcade and eng/common scripts don't pass DotNetBuildRepo in.

Copy link
Member

Choose a reason for hiding this comment

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

But if the repo is still on Arcade 8, it wouldn't be using this code at all in non-VMR builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right

Copy link
Member Author

Choose a reason for hiding this comment

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

What about a repo like Roslyn that still uses Arcade 8 and eng/common scripts from Arcade 8 inside the VMR? In source-build mode they still use their eng/common scripts which don't pass the new properties in.

Copy link
Member

Choose a reason for hiding this comment

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

The eng/common scripts aren't driving the parameters passed to the VMR-based builds, the VMR orchestrator does that. Anyways, we copy over those scripts, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyways, we copy over those scripts, right?

Not for source-build: https://github.com/dotnet/dotnet/blob/262762535e6023f91ed52f8650287a47a124faaa/repo-projects/Directory.Build.targets#L238

The eng/common scripts aren't driving the parameters passed to the VMR-based builds, the VMR orchestrator does that.

Good point.

Copy link
Member

Choose a reason for hiding this comment

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

Anyways, you can verify, but in the inner and outer builds for roslyn in the VMR, you should see all the new parameters.

@@ -114,9 +120,6 @@
<Target Name="RunInnerSourceBuildCommand"
DependsOnTargets="PrepareInnerSourceBuildRepoRoot">
<PropertyGroup>
<!-- Prevent any projects from building in the outside build: they would use prebuilts. -->
<PreventPrebuiltBuild>true</PreventPrebuiltBuild>
Copy link
Member

Choose a reason for hiding this comment

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

@dotnet/source-build-internal What happens if the outer build does build something and use pre-builts? Would they still get detected at repo-source build time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants