-
Notifications
You must be signed in to change notification settings - Fork 165
global.json updates
#71
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
Conversation
|
I think one of the scenarios that is missing here, but that could easily be addressed, is allowing the user to specify a local install directory to check. That is, for CI, some repos will always download a local SDK, regardless of what is available on box. And, while we have the ability to include additional search paths by modifying the current environment, there is no way to configure that to "just work" for a clean enlistment where the user is directly running I would propose that you should be able to specify an additional directory to check for dotnet installs as part of the As extensions, you could also have an option that blocks searching in the default locations (i.e. only look under This functionality, combined with the new functionality proposed in the OP, would allow a repository to ensure that an appropriate SDK version is used and also have that integrate with their repo infrastructure such that they don't require users to use |
|
We have this in mind for a future update, but need to keep this one simple. In other words: this is the first step of a journey. |
accepted/global-json-updates.md
Outdated
|
|
||
| When a `global.json` is encountered and it specifies an SDK version, the SDK resolvers attempt to locate that specific SDK patch version: | ||
|
|
||
| 1. When the resolver finds no `global.json` or finds one that does not specify SDK version, the latest SDK is used, regardless of whether it is preview or stable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to reflect the current behavior. Consider that when building from VS the preview SDK is only used from a preview VS.
Or is this correct in that an RTM VS will pick a preview SDK if there is no global.json but refuse to use a preview SDK if there is a global.json that specifies it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. This reflects the behavior of dotnet, but not VS. VS passes in a flag to the same underlying to control when previews are used or not. This flag defaults based on whether VS is a preview and can be overridden by a checkbox in VS. Having a bool in the global.json essentially makes this flag available without the checkbox.
So, dotnet can respect it too, and you can set up previews to be used in VS irrespective of checkboxes with this.
| * Prior to .NET Core 2.1 | ||
| * The highest patch version for the SDK feature band is used _regardless of whether it is higher than what is specified_. For example, if `global.json` specifies 2.2.103 is specified and is not available, and 2.2.101 is the highest SDK on the machine, it will be used. | ||
| * In .NET Core 2.1 and higher | ||
| * The highest patch version for the SDK feature band is used _assuming it is higher than what is specified_. For example, if `global.json` specifies 2.2.103 is specified, and 2.2.101 is the highest SDK on the machine, an error will occur. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading this correct we get the "greater than or equal to SDK version" feature by just specifying the version up to the feature brand level? That seems to be how the document reads but at the same time the behavior I'm describing has always seemed to be an ask vs. something that was already there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior today is exact version if it exists, else latest from feature band.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming @nguerrera comment resolves @jaredpar 's. This is not the same as greater or equal, which is a feature added later in this proposal.
accepted/global-json-updates.md
Outdated
| * `ignorePreview` | ||
| * `true` or `false` | ||
| * Default: `false` to match previous behavior. | ||
| * Alternate names: `releaseOnly` or `useReleaseOnly` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider also usePreview. Feel like a positive declarative property about the behavior desired here is more natural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like usePreview.
accepted/global-json-updates.md
Outdated
| * Default: `false` to match previous behavior. | ||
| * Alternate names: `releaseOnly` or `useReleaseOnly` | ||
| * This is new behavior. | ||
| * `rollForward` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the impact of declaring rollForward but not specifying sdk.version? It seems this property is only meaningful if sdk.version is specified correct?
accepted/global-json-updates.md
Outdated
| "sdk": { | ||
| "version": "3.0.100-Pre", | ||
| "ignorePreview": true, | ||
| "rollForward": "highest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be:
| "rollForward": "highest" | |
| "rollForward": "latest" |
| * This is new behavior. | ||
| * `rollForward` | ||
| * Lower version numbers are always ignored. | ||
| * `patch` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a functional difference between the following:
Option 1:
"sdk": {
"version": "3.0.1"
}Option 2
"sdk": {
"version": "3.0.100",
"rollForward": "highest"
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused. There is no 3.0.1 patch of the SDK. Is there a typo in your comment?
Co-Authored-By: Jared Parsons <jaredpparsons@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterhuene Can you review for correctness. I let this public proposal get out of date.
Thanks!
| * Prior to .NET Core 2.1 | ||
| * The highest patch version for the SDK feature band is used _regardless of whether it is higher than what is specified_. For example, if `global.json` specifies 2.2.103 is specified and is not available, and 2.2.101 is the highest SDK on the machine, it will be used. | ||
| * In .NET Core 2.1 and higher | ||
| * The highest patch version for the SDK feature band is used _assuming it is higher than what is specified_. For example, if `global.json` specifies 2.2.103 is specified, and 2.2.101 is the highest SDK on the machine, an error will occur. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming @nguerrera comment resolves @jaredpar 's. This is not the same as greater or equal, which is a feature added later in this proposal.
| * This is new behavior. | ||
| * `rollForward` | ||
| * Lower version numbers are always ignored. | ||
| * `patch` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused. There is no 3.0.1 patch of the SDK. Is there a typo in your comment?
accepted/global-json-updates.md
Outdated
| * Default: empty | ||
| * `usePreview` | ||
| * `true` or `false` | ||
| * Default: `true` to match today's behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should state that for VS, default is to honor what VS does for previews as described above. And Global.json should win over VS on this.
| ## Proposal | ||
|
|
||
| This proposal parallels [runtime roll-forward behavior as initially described](https://github.com/dotnet/core-setup/blob/master/Documentation/design-docs/roll-forward-on-no-candidate-fx.md)} and [recent improvements](https://github.com/dotnet/designs/blob/master/accepted/runtime-binding.md). All of the features proposed here may not be included in Phase 1. It will include at least `usePreview` and `"rollforward": "disable"` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing the full set below now, right. Do we need the disclaimer about phase 1?
accepted/global-json-updates.md
Outdated
| * `feature`, `minor`, `major`: highest patch of the nearest version within specified limits. | ||
| * `latestPatch`, `latestFeature`, `latestMinor`, `latestMajor`: highest patch of the highest available version within specified limits. | ||
| * `disable`: use exactly the version specified. | ||
| * `patch` **[[(I've gone back and forth on "patch" or "legacy" here)]]** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like patch. It's not exactly analogous to the other non-latest as it fallback to latest instead of falling back to nearest, but close enough. We can document the difference.
accepted/global-json-updates.md
Outdated
|
|
||
| The new behavior is that the highest patch within the feature band will be used even if the requested version is available, unless the `rollForward` is set to `disable`. | ||
|
|
||
| Also, the nearest higher SDK will be used if there is no version that matches the requested major, minor and feature. This behavior can be changed by specifying other options for the `rollForward` property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it nearest higher SDK or is it basically RollForward=major by default? The difference I see is in the case where there are multiple patches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the default should map to some well defined roll forward setting - it should not be yet another way of doing things - that's super confusing. I don't know which is the best default... currently it is basically patch. Changing it to major feels like a rather big change. But I guess we make different guarantees about SDK backward compatibility across major versions than we do for the product...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VS will be cleaning up after itself moving forward and will only have a current SDK with optional runtimes, templates, etc. for older version. As such, using recent SDK with projects targeting older runtimes is mainline scenario. We should not break it as a general rule. There will be exceptions and those can be handled with an explicit strengthening in global.json IMHO. The current strictness by default causes a mess of having to keep a bazillion sdks to open up different projects. And we're not going to be leaving SDKs behind anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be exceptions and those can be handled with an explicit strengthening in global.json IMHO.
Or you can install older SDK and major will still prefer it. In the case where you hit a break.
I do think that the sdk compatibility is different than runtime compatibility. We have always (meaning before .NET Core even) given customers latest tools (Visual Studio et al) to use on their existing projects. I think this should always work by default.
Co-Authored-By: Nick Guerrera <nicholg@microsoft.com>
Co-Authored-By: Nick Guerrera <nicholg@microsoft.com>
| * `usePreview` | ||
| * `true` or `false` | ||
| * Default: `true` to match today's behavior. | ||
| * `rollForward` (The `version` must be specified if `rollForward` is specified, except `latestMajor`.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version should be specified always if rollForward is specified. Having the latestMajor as an exception feels really weird (even though I understand that it would effectively give us the default behavior).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree only because there would be no way to represent the current default behavior if global.json were not present (or present and not have sdk settings) in global.json without allowing a version-less latestMajor. But it's not a strongly-held disagreement since I don't know how important that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is essential that we be able to represent the default behavior with an explicit entry (latestMajor with no version). This is the only way to override a setting higher in the directory structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does version=0.0.0, rollForward=latestMajor not work? I think allowing version to be omitted for latestMajor is fine, but wondering if it's true that it's impossible. Certainly using 0.0.0 is not obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I could see a scenario where someone might want some subtree to not respect the SDK settings from further up the source tree and just use the default "without a global.json" behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, version 0.0.0 would work and I agree that it would be non-obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that works, perhaps we should update the design to express the default "no global.json present" behavior with a version of 0.0.0 and treat a missing version when rollForward is specified as an error for consistency? I'm happy either way, personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree that 0.0.0 is not discoverable... I guess the original is OK then.
| * `true` or `false` | ||
| * Default: `true` to match today's behavior. | ||
| * `rollForward` (The `version` must be specified if `rollForward` is specified, except `latestMajor`.) | ||
| * There are four basic intents for roll-forward: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match the framework roll forward setting - we should explicitly state that these values are case insensitive - so I can use Patch, LatestMajor and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the implementation PR is case-sensitive, but if the runtime does it this way, the SDK should to (personally not a big fan of case-insensitive enums).
I'll fix the implementation PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should match the runtime here. Thanks @vitek-karas for the catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context: We allowed case-insensitive for framework roll forward because that setting can be specified in several different ways. For JSON it makes sense to use the same casing you have here. For command that is less obvious but might be OKish, but for environment variables it just didn't look right.
accepted/global-json-updates.md
Outdated
|
|
||
| The new behavior is that the highest patch within the feature band will be used even if the requested version is available, unless the `rollForward` is set to `disable`. | ||
|
|
||
| Also, the nearest higher SDK will be used if there is no version that matches the requested major, minor and feature. This behavior can be changed by specifying other options for the `rollForward` property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the default should map to some well defined roll forward setting - it should not be yet another way of doing things - that's super confusing. I don't know which is the best default... currently it is basically patch. Changing it to major feels like a rather big change. But I guess we make different guarantees about SDK backward compatibility across major versions than we do for the product...
accepted/global-json-updates.md
Outdated
| * The lowest SDK the resolver would select - any SDKs with lower versions are ignored. | ||
| * Identical to today - wildcards are not supported. | ||
| * Default: empty | ||
| * `usePreview` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In SemVer the correct term is pre-release. In the framework roll forward we even have an environment variable which uses the term "pre-release" for this.
I think this needs a much more detailed description of the desired behavior. Again in framework roll forward we actually spent a lot of time on this: https://github.com/dotnet/core-setup/blob/master/Documentation/design-docs/framework-version-resolution.md#pre-release-versions
In short:
- If the
versionis a release version, should we try to fulfill the request with release first and only then fallback to pre-release? (even isusePreviewis true). - What happens if
versionis pre-release butusePreview=false - If
usePreview=falseand we can't find any version and are about to fail - should we consider pre-release versions in this case to avoid failing?
In the below text you always state that we pick the latest patch in the given range (except for disable). We should be explicit what it means for pre-release versions. Do we always pick latest pre-release for a given patch? We actually went back and forth on this for frameworks and ended up having a somewhat in-between solution:
- If the
versionis pre-release, and there is an exact match, use it - don't roll to latest patch/pre-release - otherwise it's really hard to test several different pre-release versions side-by-side. Also we tend to treat pre-release versions as breaking (P4 versus P8 - there might be breaking changes there), so it's better to pick "closest" in this case, to avoid breaking people. I know that we mostly mitigate this by uninstalling other previews, but still - even for internal testing this is important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setting is allowPrerelease. This part is out of date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer questions:
If the version is a release version, should we try to fulfill the request with release first and only then fallback to pre-release? (even is usePreview is true).
If I specify version = 2.2.300, rollForward = latestMajor, allowPrerelease = true and have 2.2.505 and 3.0.100-preview7 installed, I would be quite surprised if it prefers 2.2.505 over 3.0.100-preview7. I don't think the version should have semantic meaning regarding whether or not prereleases are matched against (see next answer).
What happens if version is pre-release but usePreview=false
Currently it silently allows prerelease versions. My changes make this a warning saying we're ignoring your allowPrerelease setting and proceed to allow prereleases. Personally, I don't like either of these behaviors as to me version is a minimum bound for the roll-forward policy and shouldn't otherwise have semantic meaning.
If usePreview=false and we can't find any version and are about to fail - should we consider pre-release versions in this case to avoid failing?
I don't think so. If I instruct my tools to not consider prerelease versions (which is not the default) then it should do so.
Do we always pick latest pre-release for a given patch?
As this design is currently going to be implemented and assuming allowPrerelease = true, yes. Users should be able to explicitly use a specific prerelease patch level by using either the patch or disable policies. Generally users don't install preview servicing releases and our testers don't test multiple preview servicing releases side by side, so I think the impact here would be low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - makes sense, I just think this should be explicitly stated - if nothing else so that there's a doc which can be compared against tests/actual behavior.
| Prior to arriving at this proposal, two others were considered. Neither of these align well with roll-forward on the .NET Core Runtime. | ||
|
|
||
| * An independent versioning scheme. It's largest downside is that it would be _another_ way users would have to think about specifying versioning behavior. | ||
| * An attempt to parallel NuGet. Definining [NuGet version ranges](https://docs.microsoft.com/en-us/nuget/reference/package-versioning) is relatively complex, and it didn't feel like a good fit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also want to consider a version blacklist. A specific version is incompatible with a given project. Triggers a compile exception, generate bad code ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdmaclea Is there a specific scenario, or is this for completeness/in case we screw up.
Since we're human, this makes sense to consider for a backlog item, but I don't want to make the current round of changes any more complex unless necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for completeness or in case we screw up.
We don't have a way to work around specific broken SDK releases. It is especially problematic when the latest release is broken. Presumably the preferred solution here would be to use disable with a specific version.
Co-Authored-By: Steve MacLean <stmaclea@microsoft.com>
|
@KathleenDollard any objections to merge this so that we have a doc we can point customers at? |
Add features for