-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 suppressing transitive framework reference downloads, and other changes #25358
Support suppressing transitive framework reference downloads, and other changes #25358
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
@baronfel To review the new / updated error messages |
<comment>{StrBegin="NETSDK1184: "}</comment> | ||
</data> | ||
<data name="UnknownFrameworkReference_MauiEssentials" xml:space="preserve"> | ||
<value>NETSDK1185: This project depends on Maui Essentials through a project or NuGet package reference. To build this project, you must set the UseMauiEssentials property to true (and install the Maui workload if necessary).</value> |
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.
Looks like there's a double space after "reference. To build..."
@@ -37,6 +37,7 @@ public class GivenThatWeHaveErrorCodes | |||
1066, | |||
1101, | |||
1108, | |||
1180, |
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.
Is the a corresponding code change for the deletion?
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.
No, I think that none of these unit tests are running in CI, so this happened earlier and I just noticed when I ran the test manually.
|
||
if (s_allowCacheLookup) | ||
{ | ||
BuildEngine4?.RegisterTaskObject(cacheKey, results, RegisteredTaskObjectLifetime.AppDomain, allowEarlyCollection: true); | ||
} | ||
} | ||
|
||
foreach (var error in results.Errors) | ||
{ | ||
Log.LogError(error); |
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.
Is it worth continuing once errors have been logged?
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 don't think it hurts anything and it's the way it behaved previously.
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.
LGTM - no concerns other than a some questions I had
<comment>{StrBegin="NETSDK1184: "}</comment> | ||
</data> | ||
<data name="UnknownFrameworkReference_MauiEssentials" xml:space="preserve"> | ||
<value>NETSDK1185: This project depends on Maui Essentials through a project or NuGet package reference. To build this project, you must set the UseMauiEssentials property to true (and install the Maui workload if necessary).</value> |
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 message doesn't say why there's a problem, and the other ones do (which is a good thing). I think we can unify the tone by adding a brief section on why this is a problem.
<value>NETSDK1185: This project depends on Maui Essentials through a project or NuGet package reference. To build this project, you must set the UseMauiEssentials property to true (and install the Maui workload if necessary).</value> | |
<value>NETSDK1185: This project depends on Maui Essentials through a project or NuGet package reference, but doesn't declare that dependency explicitly. To build this project, you must set the UseMauiEssentials property to true (and install the Maui workload if necessary).</value> |
DisableTransitiveFrameworkReferenceDownloads
property. If set to true, targeting and runtime packs will not be downloaded unless there is a corresponding FrameworkReference in the project being built (instead of the default behavior, which is to download all of them because they might be needed for a transitive framework reference). This should make testing internal builds easier, as we plan to put the base runtime packs in a workload, and setting this property will mean that the ASP.NET and WindowsDesktop runtime packs won't always need to be downloaded from special build-specific feeds.Pack="false" PrivateAssets="All"
(see here), so these framework reference don't flow to referencing projects at all.EnableWindowsTargeting
property is set to true. This should help address NETSDK1100 blocks building on Linux #3592 and NETSDK1100 when building projects with multiple TargetFrameworks #23306. The reason this was disabled in the first place was because we download all targeting packs (and runtime packs for self-contained builds) for the current target framework whether they are needed or not, because they might be brought in by a transitive framework reference. We didn't want to ship the Windows targeting packs with the non-Windows SDK builds, but we also didn't want a vanilla Console or ASP.NET app to automatically download these targeting and runtime packs the first time you build. The new property enables them to only be downloaded if you opt in.