Remove VMR inner-build concept #176
Conversation
632b15f to
49725e3
Compare
754095a to
28af55e
Compare
a2c3bc4 to
609c8c6
Compare
src/arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/PrebuiltUsage.targets
Outdated
Show resolved
Hide resolved
Extracted from dotnet/dotnet#176 The above PR removes the DotNetBuild.props VMR / SB entrypoint. All settings will move into the repo infra. The RestoreUseStaticGraphEvaluation property needs to be set before Arcade's Build.proj builds the solution and in the repo infra so that dotnet/msbuild/NuGet commands use that restore mode. Therefore, also define it in eng/Build.props which is imported by Build.proj and remove the prop from the build scripts. Move the DotNetBuild condition inline into the two places that now set it.
* Move RestoreUseStaticGraphEvaluation setting inside repo Extracted from dotnet/dotnet#176 The above PR removes the DotNetBuild.props VMR / SB entrypoint. All settings will move into the repo infra. The RestoreUseStaticGraphEvaluation property needs to be set before Arcade's Build.proj builds the solution and in the repo infra so that dotnet/msbuild/NuGet commands use that restore mode. Therefore, also define it in eng/Build.props which is imported by Build.proj and remove the prop from the build scripts. Move the DotNetBuild condition inline into the two places that now set it. * Fixes and PR feedback * Add D.Solution.props to make restore consistent * Add condition and update comment * try a workaround * Revert change in editorfeatures project
2e69dfd to
fb59d8c
Compare
1. Remove the repo source-build infrastructure: - The outer/inner-build concept - The SB intermediate package creation 2. Remove the DotNetBuildPhase property and replace its usage with the now only two inclusive properties. Update docs. 3. Move the ArPow inner clone target into the VMR orchestrator and integrate it into the repo infra (still conditional only for SBE). 4. Move the ArPow target that handles creating the package usage data and the package report into the VMR orchestrator 5. Update the remaining DotNetBuild.props files with the new property, item and target names. Extra work: - Move the `WritePrebuiltUsageData` and `ReportPrebuiltUsage` targets that were executed only for dotnet.proj into finish-source-only.proj to better distinguish them from the newly added ArPow targets that got moved into the VMR orchestrator. - Place the "prebuilt-report" logs under "artifacts\log\<config>\" and avoid the now unnecessary copy step in the YML. - Remove unnecessary SourceBuild.props / DotNetBuild.props which only contained properties that were used for repo source-build. - Rename the remaining few SourceBuild.props to DotNetBuild.props
fb59d8c to
23e4c42
Compare
| <BuildArgs Condition="'$(MonoAOTEnableLLVM)' != ''">$(BuildArgs) /p:MonoAOTEnableLLVM=$(MonoAOTEnableLLVM)</BuildArgs> | ||
| <BuildArgs Condition="'$(MonoBundleLLVMOptimizer)' != ''">$(BuildArgs) /p:MonoBundleLLVMOptimizer=$(MonoBundleLLVMOptimizer)</BuildArgs> | ||
| <BuildArgs Condition="'$(PgoInstrument)' == 'true'">$(BuildArgs) $(FlagParameterPrefix)pgoinstrument</BuildArgs> | ||
| <!-- TODO: This parameter is only available on the Unix script. Intentional? --> |
There was a problem hiding this comment.
can you file an issue for this?
There was a problem hiding this comment.
That's a copy from https://github.com/dotnet/runtime/blob/dd75c45c123055baacd7aa4418f425f412797a29/eng/DotNetBuild.props#L43
I have no context on this. @tmds or @jkoritzinsky might know more
There was a problem hiding this comment.
--outputrid is runtime's name for TargetRid. I assume it is historically not set on Windows because there only portable rids are built and the runtime can infer those.
We could pass this on Windows as well (and deal with the fallout if there is any).
There was a problem hiding this comment.
The VMR already passes TargetRid in to all inner repos:
dotnet/repo-projects/Directory.Build.props
Line 116 in 025e4e5
So runtime already receives it. Maybe we just need to consolidate on TargetRid and remove OutputRid (ideally with the switch if it isn't needed)?
There was a problem hiding this comment.
Yes, we should consolidate on TargetRid.
There was a problem hiding this comment.
@tmds do you want to give that a try? You could submit your first real VMR PR and I can help with the validation :)
There was a problem hiding this comment.
It's on my TODO list. If someone beats me to it, that's fine.
|
|
||
| <BuildArgs Condition="'$(UseSystemLibs)' != ''">$(BuildArgs) /p:UseSystemLibs=$(UseSystemLibs)</BuildArgs> | ||
| <!-- Source-build will use non-portable RIDs. To build for these non-portable RID scenarios, we must do a boostrapped build. --> | ||
| <BuildArgs Condition="'$(DotNetBuildSourceOnly)' == 'true' and '$(DotNetBuildUseMonoRuntime)' != 'true'">$(BuildArgs) --bootstrap</BuildArgs> |
There was a problem hiding this comment.
we don't need a bootstrap build for mono because it doesn't execute tools like ilc during the build in that case right? might be worth a comment, or handle/ignore this in the bootstrap script itself so we don't need to have this knowledge here
There was a problem hiding this comment.
Question for @jkoritzinsky who added this to runtime: https://github.com/dotnet/runtime/blob/dd75c45c123055baacd7aa4418f425f412797a29/eng/DotNetBuild.props#L48-L49
| <!-- TODO: LinuxTracepoints --> | ||
| <BuildArgs Condition="$(UseSystemLibs.Contains('+rapidjson'))">$(BuildArgs) --cmakeargs -DCLR_CMAKE_USE_SYSTEM_RAPIDJSON=true</BuildArgs> | ||
| <BuildArgs Condition="$(UseSystemLibs.Contains('+zlib'))">$(BuildArgs) --cmakeargs -DCLR_CMAKE_USE_SYSTEM_ZLIB=true</BuildArgs> | ||
| <BuildArgs Condition="$(UseSystemLibs.Contains('-lttng'))">$(BuildArgs) /p:FeatureXplatEventSource=false</BuildArgs> |
There was a problem hiding this comment.
this should probably be
| <BuildArgs Condition="$(UseSystemLibs.Contains('-lttng'))">$(BuildArgs) /p:FeatureXplatEventSource=false</BuildArgs> | |
| <BuildArgs Condition="$(UseSystemLibs.Contains('+lttng'))">$(BuildArgs) /p:FeatureXplatEventSource=false</BuildArgs> |
There was a problem hiding this comment.
No idea. Straight copy from https://github.com/dotnet/runtime/blob/dd75c45c123055baacd7aa4418f425f412797a29/eng/DotNetBuild.props#L90
There was a problem hiding this comment.
This is meant to be '-' (minus) for not using lttng.
|
Talked with @akoeplinger offline. I will address the feedback in a follow-up: #397 |
| attempts as the install script walks through potential locations for a runtime. | ||
| The overall build script will return a proper exit code, but we don't want to pick up the printed error messages. --> | ||
| <Exec | ||
| Command="./eng/build.sh --only-build-repo-tasks -bl $(_AdditionalRepoTaskBuildArgs) $(InnerBuildArgs)" |
There was a problem hiding this comment.
@wtgodbe - IIRC this option was added to support this scenario. If that is the case perhaps this option should be cleaned up to help simplify the script. I'll let you follow-up as necessary.
Follow-up on #176 Also noticed that the YML was wrong. I tested the change in SBRP and the syntax now works. Rename PrebuiltUsage to TrackPrebuiltUsage property names and the file itself. Fix the NuGetPackageRoot cache defines in RepoLayout.props. They need to be in sync with the ones in tools.sh / tools.ps1. Source-only builds should use the repo local cache unless the NUGET_PACKAGES env var is explicitly set.
|
Here's the follow-up PR: #423 |
Fixes dotnet/source-build#4249
Fixes dotnet/source-build#3789
Fixes dotnet/source-build#3807
Fixes dotnet/source-build#4184
Fixes dotnet/source-build#4444
Fixes dotnet/source-build#5135
Extra work for which I will submitted separate PRs:
WritePrebuiltUsageDataandReportPrebuiltUsagetargets that were executed only for dotnet.proj into finish-source-only.proj to better distinguish them from the newly added ArPow targets that got moved into the VMR orchestrator: 3492e4bartifacts\log\<config>\and avoid the now unnecessary copy step in the YML: 3492e4bOfficial build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2702751&view=results (the previous official build was already green)