Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Stop making src modifications to every repo's NuGet.config #18478

Merged
merged 14 commits into from
Feb 3, 2024

Conversation

NikolaMilosavljevic
Copy link
Member

@NikolaMilosavljevic NikolaMilosavljevic commented Jan 31, 2024

Fixes: dotnet/source-build#3170

With this change, there will be no more modifications of NuGet.config files during source-build.

Few notes about implementation:

  • RestoreConfigFile property is used, for overriding resolution of NuGet.config files
  • Environment variable with the same name is set, to work enable override of NuGet.config in various repo toolset projects, i.e. in aspnetcore, fsharp and source-build-externals. It might be possible to modify the invocation of those projects to honor the MSBuild property. I've left the env.var. for now and looking for feedback on this.
  • SDK packages produced in SBRP, are utilized, to work around NuGet SDK resolver, which does not honor RestoreConfigFile property. This utilizes SourceBuiltSdkResolver functionality.
  • Infra supports multiple NuGet.config files per repo. I've preserved the support for multiple files. Some changes would have been simpler if we got rid of that support. However, it seems that only one file was used, so I'm unsure why infra supports multiple, and if we should make any changes now.
  • 5 patches are needed
  • 3 patches enable use of RestoreConfigFile property in following repos: aspnetcore, runtime, source-build-reference-packages
  • 2 patches are updating global.json files, in razor and sdk, to add the missing SDK package reference. All other repos have proper SDK references in global.json. New code, I'm adding, queries data from global.json to determine if we need to override any SDKs and which version.
  • We can only override one package with one locally restored SDK. We cannot override all package versions at once. This necessitated the aforementioned changes in global.json in 2 repos.
  • I tried to override SDKs using the latest version of the package, but that hit an error in NuGet.

Backport PRs:

@mmitche
Copy link
Member

mmitche commented Jan 31, 2024

Ooh!

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jan 31, 2024

Hmm, after rebasing to accept changes in main, I tried to do another local VMR build, which failed. It seems that arcade is built before sbrp repo. Is this an expected change?

I could likely update my changes to utilize the SDK package from PSB.

@MichaelSimons @mthalman

@mthalman
Copy link
Member

Infra supports multiple NuGet.config files per repo. I've preserved the support for multiple files. Some changes would have been simpler if we got rid of that support. However, it seems that only one file was used, so I'm unsure why infra supports multiple, and if we should make any changes now.

It only ever sets it to a single config file. In my work for the parallel builds, I've gotten rid of the ItemGroup and just using the single property. So if it makes your implementation simpler to do the same, I think you should go for it.

@mthalman
Copy link
Member

Hmm, after rebasing to accept changes in main, I tried to do another local VMR build, which failed. It seems that arcade is built before sbrp repo. Is this an expected change?

That's not what's happening in main. I'm looking at the builds in the pipeline and they are building SBRP first.

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jan 31, 2024

Hmm, after rebasing to accept changes in main, I tried to do another local VMR build, which failed. It seems that arcade is built before sbrp repo. Is this an expected change?

I could likely update my changes to utilize the SDK package from PSB.

@MichaelSimons @mthalman

This was output to console:

Building dependencies [arcade;command-line-api;sourcelink;diagnostics;emsdk;cecil;symreader;runtime;xdt;scenario-tests] needed by 'dotnet'

arcade still has a dependency on source-build-reference-packages repo, but the condition was recently changed to use a different property: https://github.com/dotnet/installer/blob/de345f1baaa42816bf4f9c40eab2e57cc1f1fb58/src/SourceBuild/content/repo-projects/arcade.proj#L9C82-L9C103

@ViktorHofer it seems that this was updated with #18402

@mthalman
Copy link
Member

arcade still has dependency on source-build-reference-packages repo, but the condition was recently changed to use a different property: https://github.com/dotnet/installer/blob/de345f1baaa42816bf4f9c40eab2e57cc1f1fb58/src/SourceBuild/content/repo-projects/arcade.proj#L9C82-L9C103

Right, the property was renamed. If it's not working in your branch, you may need to clean your environment and re-run prep.

@NikolaMilosavljevic
Copy link
Member Author

arcade still has dependency on source-build-reference-packages repo, but the condition was recently changed to use a different property: https://github.com/dotnet/installer/blob/de345f1baaa42816bf4f9c40eab2e57cc1f1fb58/src/SourceBuild/content/repo-projects/arcade.proj#L9C82-L9C103

Right, the property was renamed. If it's not working in your branch, you may need to clean your environment and re-run prep.

Thanks, I'll try that. I do see that official builds are working fine, building SBRP before arcade, so this is only an issue in my environment, which is good.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will review this more carefully tomorrow. I still haven't fully understood the direction taken here but I'm sure I will understand it tomorrow.

NikolaMilosavljevic added a commit to dotnet/razor that referenced this pull request Feb 2, 2024
Contributes to: dotnet/source-build#3170

Backport of changes in dotnet/installer#18478

### Description

Each .NET repo needs to specify all used SDKs in `global.json` file.
This change adds the missing SDK.
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. This looks much better now with single msbuild sdk versions and with the NuGetConfig simplication. The current state will break the MSFT builds so I would advise to either wait for @akoeplinger's CI PR to get merged in or do local builds on Unix and Windows with the final state of the PR (before it goes in).

@@ -4,6 +4,7 @@
},
"msbuild-sdks": {
"Microsoft.Build.NoTargets": "3.7.0",
"Microsoft.Build.Traversal": "3.4.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I logged dotnet/source-build#4085 to remove the old versions of these package from SBRP.

@NikolaMilosavljevic NikolaMilosavljevic merged commit ff1d5e5 into dotnet:main Feb 3, 2024
16 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop making src modifications to every repo's NuGet.config
5 participants