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

Explicitly reference Roslyn compiler version that's currently in the '.NET Core 3 Release' channel #16394

Merged
merged 3 commits into from
Jul 2, 2019

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Jul 2, 2019

If we decide to take this (with QB approval), I'll also add a subscription to auto-update the toolset.

@dougbu
Copy link
Member Author

dougbu commented Jul 2, 2019

No failures anywhere which means there's no need for this change in the preview 7 branch at the moment. I'm checking whether the Roslyn toolset in the Release channel will be updated soon.

/cc @ajcvickers @roji @AndriySvyryd @divega

@roji
Copy link
Member

roji commented Jul 2, 2019

OK, thanks for looking into it! Let us know and I'll finalize the PR for master in the meantime.

@ajcvickers
Copy link
Contributor

@dougbu The Roslyn people certainly indicated that this was going in preview7, and given they are locked down I would be very surprised if it isn't in. So I think this indicates that we're not yet building with the compiler that is going to ship with preview7. If that's expected, then no problem. Otherwise, I think something is wrong here.

@dougbu
Copy link
Member Author

dougbu commented Jul 2, 2019

@ajcvickers I agree something is wrong. I included you in the thread where I'm trying to confirm things so we only need to update the release/3.0-preview7 branch once.

dougbu added 2 commits July 2, 2019 11:43
…'.NET Core 3 Release' channel

- bit surprised this isn't pulling in '3.2.0-beta4-19326-12' but that may come once dotnet/roslyn branches
- see also #16370 and #16385 discussions
- also fix package name in Version.Details.xml
@dougbu dougbu force-pushed the dougbu/update.compiler branch from 8f21dec to 234d36a Compare July 2, 2019 18:44
@dougbu
Copy link
Member Author

dougbu commented Jul 2, 2019

@ajcvickers this branch now has the correct compiler toolset and I expect it to fail as expected. It may however need more changes than what @roji did for the Arcade update in 'master'. We're shipping a newer compiler toolset than what Arcade knew about. Please update my branch w/ @roji's changes if that's the right thing to do.

@dougbu
Copy link
Member Author

dougbu commented Jul 2, 2019

@wtgodbe @jac009 (I can't find Steve's GitHub alias) this is the change to bring EF Core the correct compiler version and to take the Arcade SDK out of the equation. I suggest I should do the same thing in the other ASP.NET Core repos (AspNetCore, AspNetCore-Tooling, Blazor, EntityFramework6, and Extensions) but will wait for your go-ahead.

Aside from this PR how would you like to track bringing us the latest compiler toolset?

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

LGTM, please ACK when you've added the corresponding subscription to .Net Core 3 Release

@dougbu
Copy link
Member Author

dougbu commented Jul 2, 2019

@roji previous build started to fail in the same ways as we saw in 'master'. I cherry-picked your 98d51c2 commit in hopes that covers everything but will be watching for additional nullable-related breaks. Thanks very much!

@roji
Copy link
Member

roji commented Jul 2, 2019

Sure thing. I guess this means you don't need #16403, which would apply that nullability fix directly to the preview7 branch? I (or you) can close it.

@dougbu
Copy link
Member Author

dougbu commented Jul 2, 2019

@wtgodbe:

darc add-subscription --target-repo https://github.com/aspnet/EntityFrameworkCore --channel '.NET Core 3 Release' --target-branch release/3.0-preview7 --source-repo https://github.com/dotnet/roslyn

Successfully created new subscription with id '8f534a67-affb-49bc-70eb-08d6fb34e42e'.

@dougbu
Copy link
Member Author

dougbu commented Jul 2, 2019

All tests now passing 🚀

@dougbu dougbu merged commit 37415eb into release/3.0-preview7 Jul 2, 2019
@ghost ghost deleted the dougbu/update.compiler branch July 2, 2019 21:46
@smitpatel
Copy link
Contributor

@dougbu - Is there a reason this commit is not in master branch?

@dougbu
Copy link
Member Author

dougbu commented Jul 3, 2019

Is there a reason this commit is not in master branch?

Preview 7 is a much higher priority. I will resolve any conflict's in the bot's merge PR and get this into master tonight or (more likely) tomorrow morning.

@smitpatel
Copy link
Contributor

I already merged it in master along with my PR. I just wanted to make sure if that was not intentional.

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