-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Upgrade IL.SDK version, disable generating of generate dependency file & custom message for restore tests #43471
Conversation
Tagging subscribers to this area: @safern, @ViktorHofer |
@@ -285,4 +285,15 @@ | |||
</When> | |||
</Choose> | |||
|
|||
<!-- The upfront vertical builds already generates the dependency file. --> |
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 true, but project changes that work in VS and don't require a vertical build (EG: adding a reference) would not be reflected in the deps file if we disable it. Disabling in this way will make the build seem flaky. Can you elaborate on what errors you were seeing?
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 agree with Eric. This would be a step backwards as we try to enable VS builds without an initial root build. Also disabling deps generation could easily influence test execution as xunit loads the test's deps file. I would very cautious about that.
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 debugged more and found the problem, its basically we are building all the tfms in vs but some of the project references are not build for some tfms like *-unix tfms.
i did a multiple scans over all the sln to check we didnt miss anything. There is not a good way to test this as we need a clean build for every project.
We need to add a ci protection here and a better way to catch these errors.
<Target Name="CheckVSRestoreRequirements" | ||
Condition="'$(BuildingInsideVisualStudio)' == 'true'" | ||
BeforeTargets="ResolvePackageAssets" > | ||
<Error Text="Please run dotnet restore from command line for $(MSBuildProjectName) because the project contains platform specific target frameworks." Condition="!Exists('$(ProjectAssetsFile)')" /> |
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 error text doesn't match the condition. It's saying the project has platform specific TFMs, yet the condition is only checking if the assets file is missing.
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 that sufficient? Don't you also need to disable NuGet restore in the Nuget restore settings inside VS?
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.
Right, that's required to prevent the package manager error (for the cases where it would fail) and then you probably want an error like this that tells folks to run from the commandline.
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 that sufficient? Don't you also need to disable NuGet restore in the Nuget restore settings inside VS?
i don't want to do that because that will disable the restore for all the tests that are not platform specific.
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.
Can you put a condition on the property which disables restore?
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 use a nuget.config file to disable restore for the sln and i was not able to conditionally exclude the nuget.config file from projects.
We can always add the nuget.config to the individual projects rather than libraries root but that would be a lot of duplication.
You are missing the Version.Detail.xml update for the IL Sdk. Also please remove the solution file updates from the PR. |
closing this one as the most of it is addressed in other prs |
Fixes #40159
Fixes #32205