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

Improve installation of .NET for building NuGet.Client repository #5134

Merged
merged 3 commits into from
Apr 19, 2023

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Apr 18, 2023

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/2250

Regression? Last working version:

Description

This makes it so that our hosted build always uses the .NET SDKs and runtimes we need for our build to work.

  • Moved the specification of which versions to install from MSBuild logic to a text file. This allows any machine to bootstrap .NET even if .NET isn't already installed.
  • Introduced configure.sh so non-Windows machines can initialize the .NET SDKs like Windows
  • Scripts set DOTNET_ROOT, DOTNET_MULTILEVEL_LOOKUP=0, and add dotnet to the PATH for that command window
  • You can set an environment variable, CLIBRANCHFORTESTING to use different versions for testing something else or having a different configuration for different pipelines

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@jeffkl jeffkl requested a review from a team as a code owner April 18, 2023 17:03
@jeffkl jeffkl self-assigned this Apr 18, 2023
build/CliVersions.txt Outdated Show resolved Hide resolved
dtivel
dtivel previously approved these changes Apr 18, 2023
@jeffkl jeffkl force-pushed the dev-jeffkl-dotnet-cli branch from c70298b to aacedbf Compare April 19, 2023 15:44
@jeffkl jeffkl requested a review from dtivel April 19, 2023 15:51
@@ -0,0 +1,5 @@
# Each line represents arguments for the .NET SDK installer script (https://learn.microsoft.com/dotnet/core/tools/dotnet-install-script)
-Channel 7.0
-Channel 6.0 -Runtime dotnet
Copy link
Member

@zivkan zivkan Apr 19, 2023

Choose a reason for hiding this comment

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

What's the purpose of the -runtime dotnet on the line, but not others?

We've had contributors using some IBM mainframe contribute to NuGet, but it uses the mono runtime (not Mono Project, that implements .NET Framework, but the mono runtime of the .NET Core App runtime). By specifying -runtime dotnet, is it going to cause problems for customers on platforms that the dotnet runtime doesn't have binaries for?

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 found that some of our tests need the .NET 6 runtime but don't need the full SDK. I've experimented a little and hope to trim this down as soon as all of our tests target .NET 7 or .NET 8.

@@ -26,7 +26,8 @@ Param (
[switch]$CleanCache,
Copy link
Member

Choose a reason for hiding this comment

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

Any chance while you're at it, you could move the strong name key disable code out of build.ps1 and into configure.ps1? It's something that needs to be configured once per machine, not every build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I wasn't sure if it needed to be configured unless you want to debug our VSIX? Or is it safe to disable it on everyone's machine that runs this script?

Copy link
Member

Choose a reason for hiding this comment

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

I think it needs to be disabled not only to run the VSIX, but also to run the .NET Framework restore task, the non-ilmerged nuget.exe (maybe even the ilmerged nuget.exe). I'm not sure about running tests. Maybe it's needed for that too. Basically, I think strong name validation for our 2 keys should be disabled for everyone running the script on Windows.

@jeffkl jeffkl merged commit 476dca9 into dev Apr 19, 2023
@jeffkl jeffkl deleted the dev-jeffkl-dotnet-cli branch April 19, 2023 19:13
# Each line represents arguments for the .NET SDK installer script (https://learn.microsoft.com/dotnet/core/tools/dotnet-install-script)
-Channel 7.0
-Channel 6.0 -Runtime dotnet
-Channel 5.0
Copy link
Member

@nkolev92 nkolev92 Jun 5, 2023

Choose a reason for hiding this comment

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

Why do we need the 5.0 and 3.1 SDKs?
Aren't the runtimes sufficient for this?

We'll always build with the latest installed SDK anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants