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

Use dotnet-install.sh in .NET feature #628

Merged
merged 38 commits into from
Sep 11, 2023

Conversation

sliekens
Copy link
Contributor

@sliekens sliekens commented Jul 22, 2023

Enhancements to .NET Feature and improved configuration options

This pull request builds upon the exceptional work done by @jungaretti in #467. It contains significant changes to the .NET feature to ensure better compatibility with multiple .NET versions (#389). Additionally, the installation process has been streamlined by exclusively utilizing dotnet-install.sh and discontinuing the support for installing .NET via apt. This change aims to simplify the installation process and remove the complexities associated with hybrid installations.

Major changes:

  1. New configuration options:

    • I've introduced the ability to specify a release in the form 'X.Y' or 'X.Y.Zxx'.
    • I've replaced runtimeOnly with dotnetRuntimeVersions and aspNetCoreRuntimeVersions
    • I've removed the option to specify a release in the form 'X' to align with the supported configurations of dotnet-install.sh.
  2. Valid feature configurations:
    With these changes, the following version and additionalVersions configurations are now considered valid:

    • latest (default)
    • lts
    • none
    • 3.0
    • 3.1
    • 6.0.3xx
    • 6.0.413
    • 8.0.100-preview.7.23376.3

    Additionally you may request dotnetRuntimeVersions and aspNetCoreRuntimeVersions, which use a slightly different version scheme:

    • latest
    • lts
    • 3.0
    • 3.1
    • 6.0.21
    • 8.0.0-preview.7.23375.6

    For a runtime-only installation, you can set the version to "none" to skip installing the SDK.

"features": {
    "ghcr.io/devcontainers/features/dotnet:2": {
        "version": "none",
        "dotnetRuntimeVersions": "latest, lts, 3.1",
        "aspnetCoreRuntimeVersions": "latest, lts, 3.1"
    }
}

Reasoning behind changes:

  1. Removing ability to specify only major version ('X'):
    I decided to remove the ability to specify only a major version ('X') since dotnet-install.sh does not directly support this configuration. Previously, the feature would fallback to the oldest minor version ('X.0'), which was counterintuitive. To avoid confusion and maintain clarity, I chose to remove this option and look forward to a resolution of this behavior in the dotnet-install script itself.

  2. New configuration options for runtime-only installations:
    I've replaced the runtimeOnly option with separate options for dotnet and aspnetcore runtime version for several reasons:

    • The previous setup didn't allow the installation of a mix of SDKs and Runtimes using additionalVersions, as users could either get all SDKs or all Runtimes.
    • The lack of an option to specify which runtime to install (dotnet or aspnetcore) limited user flexibility.

Testing improvements:

For comprehensive testing, I've included additional test projects, each targeting a different framework version. These test scenarios progressively utilize newer language features, such as top-level statements, implicit usings, and raw strings, in each version. The tests ensure that the example projects compile and run smoothly under the vscode remote user (except test.sh, which runs as root).

I believe these changes will significantly enhance the user experience and improve the overall stability and compatibility of the .NET feature. I welcome feedback and suggestions to continue refining and enhancing this crucial functionality. Thank you for your attention and support in reviewing this PR.

Closes #464
Fixes #293
Fixes #389

@sliekens sliekens requested a review from a team as a code owner July 22, 2023 01:49
@sliekens
Copy link
Contributor Author

@microsoft-github-policy-service agree

@sliekens
Copy link
Contributor Author

sliekens commented Jul 22, 2023

Note about using the "latest" configuration option: this will run dotnet-install.sh --version latest --channel STS which is not the same as the current latest supported release. workaround implemented

When .NET 8 is released in Nov 2023, the LTS support channel will contain the current latest supported release, until .NET 9 is released in Nov 2024, then you should use the STS support channel.

Running dotnet-install.sh without any arguments will always give you the latest LTS release. There is no option to install the current latest supported release from either STS or LTS support channels.

The implemented workaround fetches the latest.version from both channels, and chooses whichever is greater.

Another small note: it's not currently possible to pass a quality such as 'preview' or 'daily' when you specify a two-part version like '8.0'. The default quality 'GA' is implied. You can only get preview/daily builds by specifying the exact full version.

@jungaretti
Copy link
Contributor

Awesome PR. Thank you for explaining the versioning and configuration changes so clearly. This looks very similar to my PR, and I agree with its direction. Microsoft maintains dotnet-install.sh for cases like this, and I think it makes sense to leverage it as much as possible.

Also great to see you using scenario tests to prove this works for users.

I'm looking forward to hearing feedback from the Dev Containers team on the bump to v2, dotnet-install.sh direction, and tests.

@sliekens
Copy link
Contributor Author

sliekens commented Jul 26, 2023

@jungaretti I realized it's not clear from the changes that I used your script as a starting point, you can see the changes more clearly here
https://github.com/devcontainers/features/compare/jungaretti/dotnet-2..sliekens:dotnet-install-script?diff=split#diff-3dff5aa292fb61b47ab35a7440d0770b7188768ac421213739525e3aca615cdc

I see I may have accidentally removed the SHA256 verification.

My thinking is rather than verifying the SHA256 digest, wouldn't it be better to include a copy of the script in the feature? That way, upstream changes won't break the feature.

@jungaretti
Copy link
Contributor

jungaretti commented Jul 26, 2023

My thinking is rather than verifying the SHA256 digest, wouldn't it be better to include a copy of the script in the feature? That way, upstream changes won't break the feature.

Yes, this makes sense to me. Either way, we need to update the script to respond to upstream changes: updating the hash or patching the copied script. Patching the copied script is much less disruptive! and you're right, the SHA could break at any moment without warning.

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Hi 👋

Thank you so much for taking the time to contribute this spectacular change. We really appreciate it! Good job @sliekens @jungaretti 🎉

Left some thoughts/comments, let me know what you think or how I could help.

src/dotnet/install.sh Outdated Show resolved Hide resolved
src/dotnet/devcontainer-feature.json Show resolved Hide resolved
src/dotnet/devcontainer-feature.json Outdated Show resolved Hide resolved
src/dotnet/devcontainer-feature.json Outdated Show resolved Hide resolved
src/dotnet/devcontainer-feature.json Outdated Show resolved Hide resolved
src/dotnet/install.sh Outdated Show resolved Hide resolved
src/dotnet/install.sh Show resolved Hide resolved
src/dotnet/install.sh Show resolved Hide resolved
src/dotnet/install.sh Show resolved Hide resolved
test/dotnet/scenarios.json Show resolved Hide resolved
@sliekens
Copy link
Contributor Author

sliekens commented Aug 1, 2023

@samruddhikhandale @jungaretti what do you think about adding DOTNET_RUNNING_IN_CONTAINER(S) to the containerEnv?

image

https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-environment-variables

@jungaretti
Copy link
Contributor

@sliekens DOTNET_RUNNING_IN_CONTAINER(S) seems like a good decision in order to follow Microsoft's example. I have never used (or looked for) that env variable, but yes MSFT's docs specifically mention that it's set in images.

@samruddhikhandale
Copy link
Member

@samruddhikhandale @jungaretti what do you think about adding DOTNET_RUNNING_IN_CONTAINER(S) to the containerEnv?

image https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-environment-variables

Adding DOTNET_RUNNING_IN_CONTAINER=true seems fine, I can see it set in .NET sdk images. Would be nice to sync as much as possible 👍

image

@sliekens Out of curiosity, how/who uses this env variable? From a quick search, looks like this only signals a developer that .NET is running in the container and doesn't have any other use case (could be wrong)?

@sliekens
Copy link
Contributor Author

sliekens commented Aug 2, 2023

@samruddhikhandale I found an example where it is used to redirect build output to a different folder based on whether dotnet build was executed on the host or inside the container.

https://github.com/dotnet/dotnet-docker/blob/main/samples/Directory.Build.props

The reason for this is stretching the limits of my MSBuild knowledge, but, I think this configuration makes it so you can switch between running dotnet build on the host or inside the container without having to do dotnet clean in between builds.

Other than that, I think perhaps .NET itself does some things differently when this environment variable is set.

There is an open issue dotnet/dotnet-docker#3997 to document the purpose of DOTNET_RUNNING_IN_CONTAINER and I think currently @richlander is the best person to answer this question.

src/dotnet/install.sh Outdated Show resolved Hide resolved
src/dotnet/devcontainer-feature.json Show resolved Hide resolved
src/dotnet/NOTES.md Show resolved Hide resolved
src/dotnet/devcontainer-feature.json Show resolved Hide resolved
src/dotnet/install.sh Outdated Show resolved Hide resolved
src/dotnet/install.sh Outdated Show resolved Hide resolved
Copy link

@bderusha bderusha left a comment

Choose a reason for hiding this comment

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

Love the changes! This looks great. Nice work 🎉

@jungaretti
Copy link
Contributor

Agreed, looking good!

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Super cool ✨

Thank you so much for contributing this PR and reiterating based on feedbacks, we appreciate it 🚀

src/dotnet/devcontainer-feature.json Outdated Show resolved Hide resolved
Co-authored-by: Samruddhi Khandale <[email protected]>
@samruddhikhandale samruddhikhandale enabled auto-merge (squash) September 6, 2023 21:44
auto-merge was automatically disabled September 6, 2023 22:21

Head branch was pushed to by a user without write access

@sliekens
Copy link
Contributor Author

sliekens commented Sep 6, 2023

@samruddhikhandale didn't see you were trying to merge, sorry. I added the workflow for updating the vendor script 37c220f. I tested it against my fork, it created this PR 🔗sliekens#3.

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

No worries, left some minor comments. Thanks for adding the workflow

.github/workflows/update-dotnet-install-script.yml Outdated Show resolved Hide resolved
.github/workflows/update-dotnet-install-script.yml Outdated Show resolved Hide resolved
.github/workflows/update-dotnet-install-script.yml Outdated Show resolved Hide resolved
Co-authored-by: Samruddhi Khandale <[email protected]>
Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Thanks! Let me know when you are ready, I can help merge this PR if necessary.

@sliekens
Copy link
Contributor Author

I'm pretty happy with the state of this branch, thanks to everyone who reviewed and gave feedback!

@samruddhikhandale samruddhikhandale merged commit 96c1eea into devcontainers:main Sep 11, 2023
11 checks passed
@sliekens sliekens deleted the dotnet-install-script branch November 12, 2023 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants