Skip to content

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

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

Closed
wants to merge 1 commit into from

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Dec 19, 2019

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 added the Servicing-consider Shiproom approval is required for the issue label Dec 19, 2019
@wtgodbe wtgodbe requested review from Pilchie, dagood, JunTaoLuo, dougbu and a team December 19, 2019 21:49
@wtgodbe wtgodbe changed the title Don't use baseline assembly versions for RID-agnostic packages [release/3.0] Don't use baseline assembly versions for RID-agnostic packages Dec 19, 2019
Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

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

We commented on this in the team room, but just remarking here. This may be necessary, but is not sufficient to fix the issues we're having, as we think the extern being used should actually be the ref assembly version, not the implementation assembly version.

@@ -67,6 +65,12 @@
<IsPackable Condition=" '$(IsPackageInThisPatch)' != 'true' ">false</IsPackable>
</PropertyGroup>

<PropertyGroup>
<!-- When OnlyPackPlatformSpecificPackages is set, only produce packages for projects which set RuntimeIdentifier. -->
<!-- Keep this below where we set "IsPackageInThisPatch" -->
Copy link
Contributor

Choose a reason for hiding this comment

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

If moving this condition does anything, it's not because of $(IsPackageInThisPatch). That property is always true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, "almost-always true".

But, the comment remains confusing and I didn't get what you were trying to do. Best to make $(IsPackageInThisPatch) irrelevant i.e. remove the https://github.com/aspnet/AspNetCore/pull/17969/files#diff-56f8482aeb786dd105fbf93dedf2615fR78-R94 block. We should never use baseline versions except to confirm dependencies remain unchanged patch-to-patch. Every release is now complete or nearly so; the only optional shipping bits are the targeting packs.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should never use baseline versions

That was my initial thought as well, but I was a little nervous about not 100% understanding the implications. It now looks like we won't reset January for this, so I have more time to get this right.

@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 any luck w/ alternative fixes?

@jamshedd jamshedd added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jan 7, 2020
@jamshedd jamshedd added this to the 3.1.2 milestone Jan 7, 2020
@jamshedd
Copy link
Member

jamshedd commented Jan 7, 2020

Approved for Feb for 3.1.2.

@analogrelay
Copy link
Contributor

@wtgodbe can you retarget this PR to 3.1?

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 8, 2020

Closing in favor of #17970, which targets 3.1

@wtgodbe wtgodbe closed this Jan 8, 2020
@vivmishra vivmishra modified the milestones: 3.1.2, 3.1.3, 3.1.x Jan 9, 2020
@wtgodbe wtgodbe deleted the PackageInPatch branch January 5, 2021 17:21
@dougbu dougbu removed this from the 3.1.x milestone Dec 15, 2021
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.

8 participants