Skip to content

PublishData for AsyncCompletion#25913

Merged
dpoeschl merged 2 commits intodotnet:masterfrom
dpoeschl:EnableAsyncCompletionPublishing
Apr 16, 2018
Merged

PublishData for AsyncCompletion#25913
dpoeschl merged 2 commits intodotnet:masterfrom
dpoeschl:EnableAsyncCompletionPublishing

Conversation

@dpoeschl
Copy link
Copy Markdown
Contributor

@dpoeschl dpoeschl commented Apr 3, 2018

Get features/AsyncCompletion NuGets on myget

Infrastructure-only change

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

@dpoeschl dpoeschl requested a review from a team as a code owner April 3, 2018 19:31
@dpoeschl
Copy link
Copy Markdown
Contributor Author

dpoeschl commented Apr 3, 2018

@jmarolf @KirillOsenkov I'm not sure what I'm doing. Am I on the right track? How do I test before merging?

@jmarolf jmarolf changed the base branch from master to features/AsyncCompletion April 3, 2018 19:52
@jmarolf jmarolf changed the base branch from features/AsyncCompletion to master April 3, 2018 19:52
@dpoeschl dpoeschl force-pushed the EnableAsyncCompletionPublishing branch from ea08d9f to 20c8f51 Compare April 3, 2018 20:05
@dpoeschl dpoeschl requested review from a team as code owners April 3, 2018 20:05
@dpoeschl dpoeschl changed the base branch from master to features/AsyncCompletion April 3, 2018 20:07
@dpoeschl dpoeschl requested review from a team and removed request for a team April 3, 2018 20:09
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That feed might still be consumed by other teams. Please make a new feed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I verified earlier with @dpoeschl that he added a unique pre-release moniker to this branch as well. It shouldn't conflict with other packages.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no no, let's use that feed. Nobody's using it.

Copy link
Copy Markdown
Contributor Author

@dpoeschl dpoeschl Apr 3, 2018

Choose a reason for hiding this comment

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

@jasonmalinowski @KirillOsenkov - I don't have a strong opinion about which feed to use, I just want Kirill to be able to do his work and keep streams from crossing where needed. Jason seemed to think this feed is used by TypeScript (and someone else?), so Kirill are you okay with using a separate feed, or does it actually make it easier to use the existing one? Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's use this feed.

  1. We already have too many feeds in our nuget.config, and we'd have to add a new feed in 3 different places, go through PRs, CI, etc.
  2. This feed is not used by anyone currently. It was used by VS Mac at some point (to consume that version of Roslyn that's published there)
  3. This feed is ideal for this particular case. It's Roslyn for VS Mac.

Since the packages will have date-based version numbers on them, I'm curious why the concerns Jason. What kind of breakage would you expect even in theory if we just used this feed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We've verified that F# and TypeScript, both of which also used this feed, aren't using it anymore? In that case, I'm OK with it.

(I'm seeing many wrong statements on this thread which assumed only VS for Mac's repo was pointed to it. That was most definitely not the case. 😄)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And yes, people all the time see new packages on a feed, and think they should upgrade to them. If that gives them magic surprises of new experimental things, it isn't good. You laugh, but you aren't the one who gets to deal with the fallout!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes I get to deal with the fallout. Our feeds have 3.0 packages presently. I don't see how this makes the problem any worse.

Roslyn is the only repository that insists on creating multiple feeds for items like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did F# and TypeScript use our feed then? The name says VS Mac in it! I would also hope that anytime anyone updates to a newer set of Roslyn packages, they're responsible for verifying that is a good set for them and they should be prepared to deal with the consequences. Also the situation you describe seems hypothetical, while our business need is real and urgent.

Regardless of all that, we need these packages soon, please. Let's try to avoid debating further and just solve the problem of delivering us the packages.

If you have a strong preference which feed to publish them to, go ahead, I respect that. Just account for the fact that if you publish them to a different feed you will cause non-trivial work for our team, I just wanted to make sure the reasons for that extra work would be something more concrete than FUD.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did F# and TypeScript use our feed then? The name says VS Mac in it!

The work for you required us to move APIs around which created breaking changes for them. In order to do the coordinated merge of everything, they needed to prep insertions against the new surface area so we could do a simultaneous insertion of everybody's packages.

It's quite possible they've moved off. I chatted with @dpoeschl over lunch to verify we're good (or at least those teams know not to pull new packages), and then I'm OK with unblocking. We can totally use this feed, it's just not something we can do without other people knowing too.

@dpoeschl dpoeschl changed the base branch from features/AsyncCompletion to master April 4, 2018 20:11
@dpoeschl dpoeschl changed the base branch from master to features/AsyncCompletion April 4, 2018 20:11
@dpoeschl dpoeschl changed the base branch from features/AsyncCompletion to master April 4, 2018 20:15
@dpoeschl dpoeschl force-pushed the EnableAsyncCompletionPublishing branch from 8398f67 to 5294bcf Compare April 4, 2018 20:15
@dpoeschl
Copy link
Copy Markdown
Contributor Author

dpoeschl commented Apr 4, 2018

@jmarolf @jaredpar We're targeting master now. Thanks for the info there. 😄

@jasonmalinowski
Copy link
Copy Markdown
Member

TypeScript and F# have verified they're either off, or soon-to-be-off. So we're good.

@dpoeschl
Copy link
Copy Markdown
Contributor Author

dpoeschl commented Apr 4, 2018

retest windows_debug_unit32_prtest please
Reason: https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_unit32_prtest/12904/

@dpoeschl
Copy link
Copy Markdown
Contributor Author

dpoeschl commented Apr 4, 2018

retest windows_release_unit32_prtest please
Reason: https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_unit32_prtest/12888/

@dpoeschl
Copy link
Copy Markdown
Contributor Author

retest windows_debug_spanish_unit32_prtest please

@jaredpar
Copy link
Copy Markdown
Member

@dpoeschl what was the failure in the spanish node?

@dpoeschl
Copy link
Copy Markdown
Contributor Author

@jaredpar it was too stale to tell, but now it's failed again in https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_spanish_unit32_prtest/271/

@dpoeschl
Copy link
Copy Markdown
Contributor Author

retest windows_debug_spanish_unit32_prtest please

@dpoeschl dpoeschl merged commit 8bbeb8a into dotnet:master Apr 16, 2018
@jaredpar
Copy link
Copy Markdown
Member

@dpoeschl looks like a completely different flaky failure this time. Not related to Spanish run as far as i can tell.

@jaredpar
Copy link
Copy Markdown
Member

Looks like #25885

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