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

Allow more SDK versions #10130

Closed
wants to merge 1 commit into from
Closed

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Sep 16, 2020

Allows all 3.x SDK versions, starting with 3.1.300. Not sure what the minimum allowed version should be here.

@cartermp
Copy link
Contributor

So this might pass CI (I updated the global.json a while back as well), but that doesn't necessarily mean it will pass signed builds internally. Apparently the internal machines used for signed builds are behind the times and don't even have VS 16.7 installed on them yet. @brettfo or @KevinRansom can confirm.

rollForward also might not do the right thing here either. I've run into circumstances where an installed VS will ruin whatever I thought rollForward might do.

Additionally, the vs part of this file would also need updating otherwise builds would fail. My understanding is that the build servers will also not support this.

It's really shitty, I know.

@auduchinok
Copy link
Member Author

auduchinok commented Sep 16, 2020

I assumed CI uses build.cmd and it'll be fine. When using build.cmd everything worked well for me while neither Rider nor VS could open the solution since I don't have that particular SDK version installed.

@brettfo
Copy link
Member

brettfo commented Sep 16, 2020

I'm leaning away from this change for two reasons:

  1. The VMs that run the internal signed builds are very picky about what SDK is specified. While I could trigger a test build with this change, I don't think it's worth the churn, primarily because...
  2. We've been avoiding taking a dependency on the .NET 5 preview SDKs, despite division pressure to do so, because there was a break with tail calls that meant we couldn't use it. The RC1 release from a few days ago should have the fix and in another day or so we should get an automated Arcade update that pulls in the RC1 SDK, so downgrading the SDK will only buy us a few days at most until we require .NET 5.

@KevinRansom I'd like to hear your thoughts.

@KevinRansom
Copy link
Member

Let's not do this.

@auduchinok
Copy link
Member Author

The VMs that run the internal signed builds are very picky about what SDK is specified.

@brettfo Isn't it what build.cmd specifies anyways? The CI checks were all green. This change is primarily fixing local builds that contributors do, not changing what SDK is installed and used in CI.

@brettfo
Copy link
Member

brettfo commented Sep 28, 2020

There are three sets of VMs that we have to target; the public CI machines, the internal signed build VMs, and the common windows-2019 images; and for reasons (bureaucracy?) they're managed separately and updated on different schedules, so while a build may pass CI, it may not pass the internal signed build, which for business reasons, is the only one that 'matters' because that's what we use to insert into VS and the .NET SDK.

@auduchinok
Copy link
Member Author

auduchinok commented Sep 28, 2020

@brettfo Thanks for the detailed response.

the internal signed build, which for business reasons, is the only one that 'matters' because that's what we use to insert into VS and the .NET SDK.

And at the same time this particular build is something completely irrelevant to the most of open source contributors and, prior to the recent repo changes, FSharp.sln, fcs/FSharp.Compiler.Service.sln, and VisualFSharp.sln all worked great for them. I thought making community contributions feasible and easier was not that unimportant too? Looking at #10073, #10129, #10150, #10176, #10193, it seems like something has gone wrong.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Sep 29, 2020

One additional problem here, as I laid out in #10193 (which also shows a workaround for at least VS), is that the VS installer removes older versions of the SDK (dotnet/sdk#4015), which leads to some machines having it and some not. The easy way out would be to just have an option "install on first use if absent", but that option is not yet available and has been lingering since 2017 (dotnet/sdk#8254).

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.

5 participants