Document why we have -vs-deps branches #41153
Conversation
This gives some of the reasons behind why we have these branches, the alternatives we've considered and the challenges those run into.
|
|
||
| The Roslyn repo includes projects that reference Visual Studio APIs. These projects create a challenge because we have two tensions pulling us in opposite directions: | ||
|
|
||
| 1. We often want this to be able to reference the latest Visual Studio APIs, even if the APIs are not yet shipped. If the editor team creates a new API for us to implement or consume, they would |
There was a problem hiding this comment.
| 1. We often want this to be able to reference the latest Visual Studio APIs, even if the APIs are not yet shipped. If the editor team creates a new API for us to implement or consume, they would | |
| 1. We often want to be able to reference the latest Visual Studio APIs, even if the APIs are not yet shipped. If the editor team creates a new API for us to implement or consume, they would |
| back into master, or sometimes not. If we just shipped 16.4, it's possible that master-vs-deps only depends on stuff in 16.4, so we would merge master-vs-deps back to master. It's also possible that feature work | ||
| has already started on 16.5, and master-vs-deps now contains 16.5 dependent work. The "safe" choice is to merge back just the -vs-deps branch for 16.4, since we know that runs on 16.4 -- it's shipping in it! | ||
|
|
||
| # What prevents us from checking stuff what should have gone into a -vs-deps branch into a regular branch? |
There was a problem hiding this comment.
| # What prevents us from checking stuff what should have gone into a -vs-deps branch into a regular branch? | |
| # What prevents us from checking stuff that should have gone into a -vs-deps branch into a regular branch? |
| new features for Dev15. This did mean that Dev14 was still supportable, but had a bunch of limitations: | ||
|
|
||
| 1. It still didn't solve dealing with API breaks or changes within the Dev15 subset. That was still an all-or-nothing. | ||
| 2. It assumed that lightup did work well. For an entirely new, isolated feature, it was easy. For existing features, it got messier. The extreme case was where the Visual Studio platform assumined we |
There was a problem hiding this comment.
| 2. It assumed that lightup did work well. For an entirely new, isolated feature, it was easy. For existing features, it got messier. The extreme case was where the Visual Studio platform assumined we | |
| 2. It assumed that lightup did work well. For an entirely new, isolated feature, it was easy. For existing features, it got messier. The extreme case was where the Visual Studio platform assumed we |
|
|
||
| master is not the only branch that has -vs-deps branches. We maintain the pair for all our milestone branches. Consider the case where we are beginning to lock down a release. At this time we often | ||
| snap master and only approved changes go in. We still have a -vs-deps branch and a regular branch, if for no other reason than we must auto-merge any further changes into the appropriate branches. Why | ||
| not branch _just_ the -vs-deps branch? Because then all changes would go into there, and a compiler change which has no reason to be anywhere in -vs-deps would end up getting merged into master-vs-deps. |
There was a problem hiding this comment.
This doesn't sound quite right. AFAIK, the reason is that 16.X-previewY has not been shipped, so we cannot merge 16.X-previewY-vs-deps into the non-vs-deps branch if we want it publicly buildable. We can't merge everything into the -vs-deps branch because then the merges will only put changes into the 16.X-previewY+1-vs-deps branch, instead of the non-VS-deps branch it belongs in.
|
|
||
| # Isn't this all a pain? Can we get rid of the -vs-deps branches? | ||
|
|
||
| Maybe, but it potentially replaces one set of problems with another set of problems. So far, the pain of this approach has been the lesser pain, so we've stuck with it. Here's some of the |
There was a problem hiding this comment.
Maybe a better question is -- can we create -vs-deps branches on-demand? Does every release need one? Why does every release need one (i.e., is this essential churn, or is there some debt which is causing us to adapt to new APIs constantly).
Very much agree. This is a great document that helps elaborate what trade offs we made here and what we'd be giving up or need to re-implement if we moved to a new model. |
|
|
||
| 1. We often want this to be able to reference the latest Visual Studio APIs, even if the APIs are not yet shipped. If the editor team creates a new API for us to implement or consume, they would | ||
| like us to consume it as soon as they have merged their feature work into the (internal) Visual Studio master branch; this means they can get API feedback and shake out any bugs as soon as possible. | ||
| 2. We want public contributors to be able to contribute to all of Roslyn, the Visual Studio layers included. Even if the contributor isn't _changing_ the code in the Visual Studio layer, deploying to Visual |
There was a problem hiding this comment.
I'd say another tension here, at least one that has been more prominently recently, is we want to minimize the time and distance (work) between when a change is committed to master and when it gets validated by the Visual Studio validation systems (RPS for example).
I'm not sure it quite rises to the level of the points you have here though.
There was a problem hiding this comment.
Well, right now that's the tension that's ignored entirely with this approach. :-/
|
|
||
| # What prevents us from checking stuff what should have gone into a -vs-deps branch into a regular branch? | ||
|
|
||
| Integration tests; our integration tests in master are running against released bits, and not random nightly builds. The common case of us moving to a new API in a mainline scenario would break integration tests. Obviously |
There was a problem hiding this comment.
When discussing integration tests I think we need to call out that we never run integration tests against non-public previews of Visual Studio. The reason being that it's not public hence it can't be used in a GitHub based build. This means that -vs-deps branches can lack coverage compared to say master. This is important to keep in mind when we discuss items like removing -vs-deps branches because one of the consequences is that in some cases we completely lose integration test coverage.
| When we were trying to support Dev14 and Dev15 at the same time, Roslyn did a model where we had two VSIXes; there was one that had most of our stuff that ran on Dev14 and Dev15, and one that had the | ||
| new features for Dev15. This did mean that Dev14 was still supportable, but had a bunch of limitations: | ||
|
|
||
| 1. It still didn't solve dealing with API breaks or changes within the Dev15 subset. That was still an all-or-nothing. |
There was a problem hiding this comment.
This sentence somehow doesn't quite carry the weight of the problem it's describing. Once realized they were going to change APIs within the "shipped" subset the splitting between VSIX ideas was pretty much "game over man".
Basically this point renders the rest a bit moot cause it's such a death nail to this approach.
|
Are we planning on merging this doc soon-ish? I understand we're in the process of considering removing the -vs-deps infra. That seems completely reasonable. It would be nice though to have this doc checked in before that, even if we just delete it as part of the change to remove -vs-deps. It's a good historical record of why we used to do things and could be valuable for anyone who has to come back and do servicing one day. |
It must be interesting to read how it was removed. 😄 |
|
At this point I wouldn't ask anyone to go write a doc like this (not sure it's wort the cost). But given we have such a doc ready I think it's valuable to put into the history for future developer archeologist to find. Further if we ever decide to revisit the decision we have a doc outlining the previous benefits / downsides it provided. |
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
I think it's a good idea to merge this and have it around, perhaps with a small addendum at the top indicating the dates it was written and the releases it applied to at the time. For example, "This was how Roslyn was shipped for many Visual Studio releases until at least dev17.0."
| Studio is the only way to actually _test_ any changes made to any of our layers in the middle in an interactive way. A contributor making a change to a code fix or refactoring can of course write unit | ||
| tests, but they can't just "try it locally" to experiment. | ||
|
|
||
| To find a balance between these two tensions, we have two branches. Using our master branch as an example, we have two branches named 'master' and 'master-vs-deps': |
There was a problem hiding this comment.
main-vs-deps is now gone :)
Not sure about "specificRelease"-vs-deps. Will those no longer be created too?
There was a problem hiding this comment.
We plan to avoid -vs-deps most of the time and recreate it temporarily if it is needed.
|
@jasonmalinowski @jmarolf What do you think our next steps should be with this PR? Especially in the context of us (i think) no longer wanting to have vs-deps branches anymore. |
I think moving some of the stuff from the oneNote to github would be a good thing. I would be fine merging with some changes to discuss that is it here for posterity |
|
@jmarolf @CyrusNajmabadi Yeah, I think we're best off updating this with the new policy and then merging it. Ironically, I think the last two minor releases for VS we've had to bring back -vs-deps both times, so I don't think it's entirely going away soon. (Alas...) |
11 participants
This gives some of the reasons behind why we have these branches, the alternatives we've considered and the challenges those run into. This is not to be used as a justification of why we cannot move away from a -vs-deps model, but rather the new problems we create if we do so. The concerns of course may change over time, and also be solvable in other ways we haven't thought of, and this document should not shut those conversations down. We just want to capture past conversations so we don't forget them.