-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix _AssemblyInTargetingPack value during servicing #95278
Conversation
The _AssemblyInTargetingPack property in packaging.targets depends on `IsNETCoreAppSrc` or `IsNetCoreAppRef` which aren't defined until src/libraries/Directory.Build.targets is imported. As packaging.targets is imported first, there's a property sequencing issue. The fix is to move the ´_AssemblyInTargetingPack` logic out of packaging.targets as that's code that is specific to targeting pack libraries which reside under src/libraries. This fixes the issue that appeared in the release/8.0 branch for main.
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsThe _AssemblyInTargetingPack property in packaging.targets depends on As packaging.targets is imported first, there's a property sequencing issue. The fix is to move the ´_AssemblyInTargetingPack` logic out of packaging.targets as that's code that is specific to targeting pack libraries which reside under src/libraries. This fixes the issue that appeared in the release/8.0 branch for main.
|
<_IsWindowsDesktopApp Condition="$(WindowsDesktopCoreAppLibrary.Contains('$(AssemblyName);'))">true</_IsWindowsDesktopApp> | ||
<_IsAspNetCoreApp Condition="$(AspNetCoreAppLibrary.Contains('$(AssemblyName);'))">true</_IsAspNetCoreApp> | ||
<_AssemblyInTargetingPack Condition="('$(IsNETCoreAppSrc)' == 'true' or '$(IsNetCoreAppRef)' == 'true' or '$(_IsAspNetCoreApp)' == 'true' or '$(_IsWindowsDesktopApp)' == 'true') and '$(TargetFrameworkIdentifier)' != '.NETFramework'">true</_AssemblyInTargetingPack> | ||
<AssemblyVersion Condition="'$(_AssemblyInTargetingPack)' != 'true'">$(MajorVersion).$(MinorVersion).0.$(ServicingVersion)</AssemblyVersion> |
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.
Will this start applying the change to ref
projects as well - and have we validated it does so correctly? I see we previously conditioned inclusion of packaging.targets
on IsPackable
which was only set in src
projects and now this will apply to both src
and ref
. I haven't spotted a problem with that right now, and changing the version in both is the right thing. Just double check that things are working correctly with ref
projects.
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.
Good point, thanks for catching this. This logic also relies on the ServicingVersion
property which is defaultedin packaging.targets and only changed in source projects, so only for packable projects. I think it's best to keep the conditions on this that we had previously. Any objection to that?
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.
Out of curiosity - What kinds of problems will you look for in the ref projects, @ViktorHofer? What things could break with this change?
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.
With the previous state of this PR, reference source projects would have had the following assembly version: 9.0.0.
during servicing. So the last servicing digit would have been missing because ServicingVersion
wasn't defined.
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.
Assuming no problem found with ref projects, lgtm.
LGTM With the |
The _AssemblyInTargetingPack property in packaging.targets depends on
IsNETCoreAppSrc
orIsNetCoreAppRef
which aren't defined until src/libraries/Directory.Build.targets is imported.As packaging.targets is imported first, there's a property sequencing issue.
The fix is to move the
_AssemblyInTargetingPack
logic out of packaging.targets as that's code that is specific to targeting pack libraries which reside under src/libraries.This fixes the issue that appeared in the release/8.0 branch in main.