Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
NikolaMilosavljevic merged 14 commits into from
Feb 3, 2024
Merged

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

NikolaMilosavljevic merged 14 commits into from
Feb 3, 2024

Conversation

@NikolaMilosavljevic
Copy link
Member

@NikolaMilosavljevic NikolaMilosavljevic commented Jan 31, 2024 •
edited
Loading

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:

@NikolaMilosavljevic NikolaMilosavljevic requested a review from a team as a code owner January 31, 2024 17:53
This was referenced Jan 31, 2024
Merged
Merged
ViktorHofer
ViktorHofer reviewed Jan 31, 2024
View reviewed changes
@mmitche
Copy link
Member

mmitche commented Jan 31, 2024

Ooh!

ViktorHofer
ViktorHofer reviewed Jan 31, 2024
View reviewed changes
@NikolaMilosavljevic NikolaMilosavljevic mentioned this pull request Jan 31, 2024
Merged
@NikolaMilosavljevic NikolaMilosavljevic mentioned this pull request Jan 31, 2024
Merged
ViktorHofer
ViktorHofer reviewed Jan 31, 2024
View reviewed changes
ViktorHofer
ViktorHofer reviewed Jan 31, 2024
View reviewed changes
@NikolaMilosavljevic NikolaMilosavljevic mentioned this pull request Jan 31, 2024
Merged
@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jan 31, 2024 •
edited
Loading

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

mthalman commented Jan 31, 2024

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

mthalman 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?

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 •
edited
Loading

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

mthalman commented Jan 31, 2024

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

NikolaMilosavljevic commented Jan 31, 2024

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.

ViktorHofer
ViktorHofer reviewed Jan 31, 2024
View reviewed changes
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 NikolaMilosavljevic mentioned this pull request Jan 31, 2024
Closed
MichaelSimons
MichaelSimons reviewed Jan 31, 2024
View reviewed changes
mmitche
mmitche reviewed Feb 1, 2024
View reviewed changes
NikolaMilosavljevic added a commit to dotnet/razor that referenced this pull request Feb 2, 2024
@NikolaMilosavljevic
Add missing SDK dependency to global.json (#9886)
2f51e1d 
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.
ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
Copy link
Member

@ViktorHofer ViktorHofer left a comment •
edited
Loading

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).

ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 2, 2024
View reviewed changes
MichaelSimons
MichaelSimons reviewed Feb 2, 2024
View reviewed changes
@MichaelSimons MichaelSimons mentioned this pull request Feb 2, 2024
Closed
MichaelSimons
MichaelSimons reviewed Feb 2, 2024
View reviewed changes
src/SourceBuild/content/global.json
},
"msbuild-sdks": {
"Microsoft.Build.NoTargets": "3.7.0",
"Microsoft.Build.Traversal": "3.4.0",
Copy link
Member

@MichaelSimons MichaelSimons Feb 2, 2024

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
@ellahathaway ellahathaway mentioned this pull request Feb 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mmitche mmitche mmitche left review comments

@mthalman mthalman mthalman left review comments

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@MichaelSimons MichaelSimons MichaelSimons approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@NikolaMilosavljevic @mmitche @mthalman @ViktorHofer @MichaelSimons