Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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 surprised to see
$(NetPrevious)
in that list. Shouldn't roslyn's source-built product (built inside the VMR) only targetNetCurrent
? cc @MichaelSimonsThere 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.
deferring to @mthalman and @jaredpar who worked on this pattern.
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.
cc @jasonmalinowski
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.
Every time we've tried this in the past we've ended up breaking some other part of the build (there is always an older runtime or dependency hanging around that forces us into
$(NetPrevious)
). This is why we keep both current + prev even when in full source build. If you all are confident this no longer holds we can remove the all the$(NetPrevious)
from this<When>
block ... but I will small stakes bet we will find some way this breaks us again 😉Alternatively if we want to keep building current + previous I think the right fix is to change the project files so that they agree on TFM as they depend on each other:
Basically make the latter
$(NetRoslynBuildHostNetCoreVersion)
Uh oh!
There was an error while loading. Please reload this page.
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.
Note that NetRoslynBuildHostNetCoreVersion is used as TargetFramework (no plural) here
roslyn/src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj
Line 101 in c8fba92
so that will need to be updated somehow if NetRoslynBuildHostNetCoreVersion is changed to contain multiple values. (Perhaps
.Split(';')[0]
or something like that would work.)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 programming should is a four letter word. ;)
Again, I feel like every time we try this we convince ourselves it's safe, find out that in practice it's not and we revert. I'm happy to take the bet again but it's more up to you all.
I'm hopeful that once we get VMR source build working that we can eliminate repo source build entirely. It seems to be of diminishing value given how quickly we get full VMR validation post merge.
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 were actually considering the opposite by adding another repo validation leg to mimic the product build (VMR) when not building from source. I'm not sure whether we would prefer to find breaks post-merge during insertion from roslyn -> sdk. I admit that the current (source-build) validation leg doesn't validate all important cases which is why I asked above whether it should target the newer TFM and run with the newer Arcade SDK and .NET SDK.
cc @mmitche @MichaelSimons
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.
My mental model is that we have repo source build because the time between when a change merges and when we get full source build validation is very long. For a normal change the validation is probably ~3 days, and pretty regularly it can week(s). It's also hard to spot test a change because you really need an official build + darc flow to do a source build test.
None of that is true with VMR source build. The time between merging a change and getting full source build validation is hours. That is because we flow code not binaries. So the moment we merge the flow should happen to VMR. Also as a dev if I think a change is source build risky it's trivial to validate locally. Just apply my change to VMR, run the source build leg (can even put up a PR) and I will get validation.
At that point I would push back on the need for repo local validation of source build.
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 forwarded your message to our internal communication channel. Thanks for providing your input. Now is the best time to share opinions as we haven't started the repo level validation epic yet. We will discuss this in length (presumably in our next sync meeting) and get back to you.
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.
Happy to come to a sync and elaborate in detail about this.