Skip to content

[release/3.1] Don't use baseline assembly versions for RID-agnostic packages #17970

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

Merged
merged 1 commit into from
Jan 15, 2020

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Dec 19, 2019

3.1 port of #17969

In all CI builds except Windows x64, we set OnlyPackPlatformSpecificPackages=true: https://github.com/aspnet/AspNetCore/blob/e6af4bfe7e022e2c3b32c583b280965041c3d15c/.azure/pipelines/ci.yml#L254-L261

This makes us decide that all libraries without RuntimeIdentifiers are not packable: https://github.com/aspnet/AspNetCore/blob/1ff3e7b21ea5270b832a6e05c74b1b5f67d4cfb0/Directory.Build.targets#L5

Then later, we say that everything that is not packable, is not part of "this patch" (if we're doing a patch build): https://github.com/aspnet/AspNetCore/blob/1ff3e7b21ea5270b832a6e05c74b1b5f67d4cfb0/Directory.Build.targets#L62-L68

Which causes us to use basline assembly versions:

https://github.com/aspnet/AspNetCore/blob/1ff3e7b21ea5270b832a6e05c74b1b5f67d4cfb0/Directory.Build.targets#L74-L77

We are incorrect to say that these libraries/packages aren't "in this patch" - we're just not building them right now. Moving the condition for OnlyPackPlatformSpecificPackages down in the file should fix the issue. Without this, all of our non-Windows-x64 builds create runtime packs w/ 3.0.0.0 assembly versioned .dll's.

@JunTaoLuo @Pilchie @dagood @dougbu PTAL

@wtgodbe wtgodbe changed the title Don't use baseline assembly versions for RID-agnostic packages [release/3.1] Don't use baseline assembly versions for RID-agnostic packages Dec 19, 2019
@dougbu
Copy link
Contributor

dougbu commented Dec 20, 2019

See my comments in #17969

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Dec 20, 2019
@dougbu
Copy link
Contributor

dougbu commented Jan 2, 2020

@wtgodbe am I correct you're waiting to resolve questions in #17969 before turning to this one?

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 8, 2020

This was approved for 3.1.2 - see #17969

@wtgodbe wtgodbe added the Servicing-approved Shiproom has approved the issue label Jan 8, 2020
@wtgodbe wtgodbe added this to the 3.1.2 milestone Jan 8, 2020
@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 9, 2020

Watching https://dev.azure.com/dnceng/public/_build/results?buildId=476946 to see if the test failures are unique to this PR

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 9, 2020

Hmmm - this might not be needed in 3.1. I just looked at the runtime packs for 3.1.1, and they don't exhibit the same symptom as the 3.0.2 runtime pack (that is, all AssemblyVersions are correct at 3.1.1.0, where in 3.0.2, the non-x64 runtime packs all have AssemblyVersions 3.0.0.0). Will investigate why this is the case tomorrow.

@vivmishra vivmishra modified the milestones: 3.1.2, 3.1.3 Jan 9, 2020
@vivmishra
Copy link

vivmishra commented Jan 9, 2020

Moved to Mar as per Tactics. Will need to be explicitly approved for Feb, if required.

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 9, 2020

Confirmed locally that this fixes the assembly versions. The Helix failures are not new, see https://dev.azure.com/dnceng/public/_build/results?buildId=476946&view=ms.vss-test-web.build-test-results-tab (note than I ran this PR's Helix tests 4 times, resulting in the 1163 failures, which is almost exactly 291 * 4). I tried to remove the IsPackageInThisPatch concept entirely, but it gave errors that I'm not confident in the fix to - considering punting that improvement to March given the high bar for the Feb release - @dougbu thoughts?

@dougbu
Copy link
Contributor

dougbu commented Jan 9, 2020

considering punting that improvement to March given the high bar for the Feb release

Agreed

@dougbu
Copy link
Contributor

dougbu commented Jan 9, 2020

@Pilchie how do we ask for this to be approved for Feb.?

@dougbu
Copy link
Contributor

dougbu commented Jan 9, 2020

(if it's more than the list you emailed around 😈)

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 10, 2020

Worth noting that the original bug here is a bit mysterious - it seems that all projects with netstandard configurations are affected, but we haven't been able to get those projects to build correctly by removing their netstandard configurations. A follow up here should be to investigate why that was the case. It seems that the _InitializeAssemblyVersion target in Arcade is returning both an AssemblyVersion and FileVersion for the unaffected projects, while only returning a FileVersion for the affected projects. Our default AssemblyVersion is incorrect, the one from the target is correct.

https://github.com/dotnet/arcade/blob/d4a1ce6278134f5dc25843e228d0498203031e61/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.targets#L14

dougbu added a commit that referenced this pull request Jan 16, 2020
@JunTaoLuo JunTaoLuo mentioned this pull request Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants