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

[release/6.0] Increment source generator assembly versions with every build #58919

Merged
merged 4 commits into from
Sep 15, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 10, 2021

Backport of #58836 to release/6.0

/cc @Anipik

Customer Impact

Not incrementing the version number can be problematic for VBCSCompiler during reference package updates, leading to dramatic reduction in compiler throughput (for Roslyn.sln, builds are about 3x slower).

Testing

Minimal

Risk

low. Roslyn analyser already uses these guidelines for assembly version updates for analyzers.

@ghost
Copy link

ghost commented Sep 10, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #58836 to release/6.0

/cc @Anipik

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@Anipik
Copy link
Contributor

Anipik commented Sep 10, 2021

cc @danmoseley

@danmoseley
Copy link
Member

Testing

Minimal

😄 ... do we have enough testing? what could go wrong? have we tested this locally?
cc @eerhardt

@eerhardt
Copy link
Member

Looks like you are going to need #58220 in 6.0 as well.

@Anipik
Copy link
Contributor

Anipik commented Sep 10, 2021

do we have enough testing? what could go wrong? have we tested this locally ?

not sure what kind of testing we need here. I checked the assembly versions, and they are being correctly updated. @eerhardt @sharwell

what could go wrong?

Roslyn analysers have been shipping them with this flag turned for a while now. generators are self contained so i don't expect to leak the assembly version to someplace else. The mitigation in case of a low probability event can be handled by pinning to a higher version.

@danmoseley
Copy link
Member

Approved (if necessary with cherry pick of change @eerhardt mentioned). Reasoning : best practice guidance from Roslyn team expected to have significant performance effect in a scenario not meeting perf goal for GA.

@danmoseley danmoseley added the Servicing-approved Approved for servicing release label Sep 11, 2021
@ericstj
Copy link
Member

ericstj commented Sep 13, 2021

not sure what kind of testing we need here. I checked the assembly versions, and they are being correctly updated

@Anipik we mentioned that we'd want to double check assembly version consistency in the build. Examine the a) nuget package, b) ref-pack/aspnet-transport package, c) ref pack file list from the official build to make sure they agree. We believe they should, but since this is the first time we're using arcade's auto-assembly version we want to make sure our build is setup correctly to ensure consistency between legs.

* Replacement tokens in logging test baseline files

Instead of using a hardcoded version of the assembly version
on the logging generated baseline files used by tests we use
replacement tokens and at runtime we replace them.

fixes #58175
@Anipik Anipik merged commit c705b70 into release/6.0 Sep 15, 2021
@jkotas jkotas deleted the backport/pr-58836-to-release/6.0 branch September 18, 2021 04:09
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants