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

Upgrade IL.SDK version, disable generating of generate dependency file & custom message for restore tests #43471

Closed
wants to merge 2 commits into from

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Oct 15, 2020

Fixes #40159
Fixes #32205

  • Upgrade Microsoft.NET.Sdk.IL to get CompileDesignTime target.
  • Added CheckVSRestoreRequirements for build being failing due to restore not being done correctly for tfms with platforms. For all other projects restore from the VS will succeed and error wont be triggered.
  • setting GenerateDependencyFile to false. A couple of projects were failing in this target due to some references being not set properly for unix configurations. The design time build provide some information but it was not enough to drill into. However, just adding this condition resolve this error, and we are able to run and build all the projects.
  • closing the duplicate runtime issue because the message needs to be fixed by the nuget. We have added the suitable features so that this error does not have any effect on the vs experience. As the restore is not run as a part of target, we cannot edit the error message.

@ghost
Copy link

ghost commented Oct 15, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

@Anipik Anipik requested review from ericstj and ViktorHofer October 15, 2020 23:08
@@ -285,4 +285,15 @@
</When>
</Choose>

<!-- The upfront vertical builds already generates the dependency file. -->
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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)')" />
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

@Anipik Anipik Oct 16, 2020

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.

@ViktorHofer
Copy link
Member

You are missing the Version.Detail.xml update for the IL Sdk. Also please remove the solution file updates from the PR.

@Anipik
Copy link
Contributor Author

Anipik commented Nov 19, 2020

closing this one as the most of it is addressed in other prs

@Anipik Anipik closed this Nov 19, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2020
@Anipik Anipik deleted the restore branch January 30, 2021 21:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build for System.Runtime fails within VS Remove duplicates From the TargetFrameworks while doing restore.
4 participants