-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Only build applicable build Tasks when building the repo #104226
Conversation
We're starting to build too many tasks unconditionally.
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
Related to #57923 (cc @ViktorHofer). |
src/tasks/tasks.proj
Outdated
<ProjectReference Include="$(MSBuildThisFileDirectory)**\*.csproj" /> | ||
<MonoTaskProject Include="AndroidAppBuilder\AndroidAppBuilder.csproj" /> | ||
<MonoTaskProject Include="AotCompilerTask\MonoAOTCompiler.csproj" /> | ||
<MonoTaskProject Include="AppleAppBuilder\AppleAppBuilder.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do have NativeAOT support on Apple/WASM so I don't think grouping these as MonoTaskProject
makes sense.
I'd rather do the split by OS, but I'm not sure if the workloads build throws a wrench into that. @steveisok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A TargetsMobile check might be appropriate here. @akoeplinger that might satisfy the official build issue we talked about yesterday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree the item name shouldn't contain mono.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A TargetsMobile check might be appropriate here. @akoeplinger that might satisfy the official build issue we talked about yesterday.
FWIW there is a new official build for 8.0.8 which ran yesterday that did not have a retry. The produced MonoTargets.Sdk packages and the workload contents are matching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ViktorHofer @akoeplinger tasks.proj seems to run too early to pick up TargetsMobile
that's defined in the top level Directory.Build.props
. Any suggestions besides duplicating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an msbuild evaluation order perspective, the
TargetsMobile
property should be available in tasks.proj.
For whatever reason, it's not available. I can't tell why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit I just pushed will have CI runs that show it not being resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sigh, maybe the difference is that it's a new day ;-). Glad we don't need to do something else then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of the needed tweak to the item name (I'm terrible with names), this is good to go!
@akoeplinger should we try to change: runtime/src/mono/nuget/mono-packages.proj Lines 34 to 36 in 2e585aa
to be conditioned on TargetsMobile as part of this PR?If we don't, building with +packs subset will build the package even if we are not targeting mobile.
|
@ivanpovazan yeah sounds good I think |
src/tasks/tasks.proj
Outdated
<ProjectReference Include="$(MSBuildThisFileDirectory)**\*.csproj" /> | ||
<MonoTaskProject Include="AndroidAppBuilder\AndroidAppBuilder.csproj" /> | ||
<MonoTaskProject Include="AotCompilerTask\MonoAOTCompiler.csproj" /> | ||
<MonoTaskProject Include="AppleAppBuilder\AppleAppBuilder.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we should try to avoid duplicating it
src/tasks/tasks.proj
Outdated
|
||
<ItemGroup Condition="'$(TargetsMobile)' == 'true'"> | ||
<MonoTaskProject Include="AndroidAppBuilder\AndroidAppBuilder.csproj" /> | ||
<MonoTaskProject Include="AotCompilerTask\MonoAOTCompiler.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is used in desktop mono AOT tests I think, we should move it outside of TargetsMobile
@MichalStrehovsky how should we proceed with this PR? The comment I made in: #104226 (comment) would potentially solve an issue we are having with the official builds. |
I just came back from vacation today and I'm happy people moved it forward while I was gone. If we're happy with the extent of testing done in CI and people are happy with how this looks like, I'm fine with merging this. |
I just realized that comment is proposing more changes that are not part of this PR yet. Is the proposed change related in any way? Should that be a separate PR? |
Welcome back!
It is partly, as we have an issue with over-building build Tasks packages. I will open a separate PR. |
I got rid of the MonoTaskProject indirection since it doesn't make sense with the TargetsMobile condition and no RuntimeFlavor=mono restriction anyway. ProjectReference it is. |
…net#104226)" This reverts commit 61050ae.
We're starting to build too many tasks unconditionally and it's observable.
Opening as draft, just want to see what the CI thinks first.