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

Include all .xml files in targeting pack #26146

Merged
merged 5 commits into from
Oct 13, 2020
Merged

Include all .xml files in targeting pack #26146

merged 5 commits into from
Oct 13, 2020

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Sep 21, 2020

Resolves #26068 and the 3.1 part of #26073. We were intentionally including the .xml files for project references & Extensions references, but not other package references. Also turns on the build of the targeting pack for next month's 3.1 servicing event.

@wtgodbe wtgodbe added the tell-mode Indicates a PR which is being merged during tell-mode label Sep 21, 2020
@wtgodbe wtgodbe added this to the 3.1.10 milestone Sep 21, 2020
@wtgodbe wtgodbe requested a review from a team September 21, 2020 20:44
@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 21, 2020

CI won't build the ref pack, this test build will: https://dev.azure.com/dnceng/internal/_build/results?buildId=824004&view=results

@Pilchie
Copy link
Member

Pilchie commented Sep 21, 2020

👀

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 21, 2020

@Pilchie think it's worth trying to sneak this into 3.1.9? The build is currently at corefx. That said, this bug has existed since at least 3.0.0, so it's not necessarily urgent that we fix it today.

@Pilchie
Copy link
Member

Pilchie commented Sep 21, 2020

Agree that it's not urgent. I'm fine with including it if it doesn't introduce any risk, but it's not worth pushing hard for.

<IsTargetingPackBuilding Condition=" '$(DotNetBuildFromSource)' == 'true' ">false</IsTargetingPackBuilding>
<IsTargetingPackBuilding
Condition=" '$(IsTargetingPackBuilding)' == '' AND '$(AspNetCorePatchVersion)' != '8' ">false</IsTargetingPackBuilding>
Condition=" '$(IsTargetingPackBuilding)' == '' AND '$(AspNetCorePatchVersion)' != '10' ">false</IsTargetingPackBuilding>
Copy link
Member

Choose a reason for hiding this comment

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

Insert swear words here

Copy link
Member Author

Choose a reason for hiding this comment

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

😭

Copy link
Member

Choose a reason for hiding this comment

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

I messed up the top-level discussion. Instead, update changes to this file and Versions.props to target 3.1.9.


<RefPackContent Include="@(AspNetCoreReferenceAssemblyPath)" PackagePath="$(RefAssemblyPackagePath)" />
<RefPackContent Include="@(AspNetCoreReferenceDocXml)" PackagePath="$(RefAssemblyPackagePath)" />
<RefPackContent Include="@(AspNetCoreReferenceDocXml->Distinct())" PackagePath="$(RefAssemblyPackagePath)" />
Copy link
Member

Choose a reason for hiding this comment

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

I'm less concerned about duplicate XML doc file paths than duplicate files in the final layout i.e. different doc files depending on which is copied second. Suggest copying %(Filename) from this item group into a temporary one and adding an <Error/> if its ->Count() doesn't match ->Distinct()->Count().

Copy link
Member Author

Choose a reason for hiding this comment

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

There will be an error anyways if there are duplicates, which will have a more descriptive error message than our custom error, since it lists the actual duplicate (similar to the .pdb issue).

@dougbu
Copy link
Member

dougbu commented Sep 21, 2020

CI won't build the ref pack

Huh❔ CI would have built the Microsoft.AspNetCore.App.Ref package if you had not changed how $(IsTargetingPackBuilding) is set. Revert that and this PR will build fine for 3.1.9.

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 21, 2020

Huh❔ CI would have built the Microsoft.AspNetCore.App.Ref package if you had not changed how $(IsTargetingPackBuilding) is set. Revert that and this PR will build fine for 3.1.9.

I targeted 3.1.10 because the branch is closed for 3.1.9 changes. We'd need to get tactics approval to merge this late, which doesn't seem worth it.

@dougbu
Copy link
Member

dougbu commented Sep 21, 2020

We'd need to get tactics approval to merge this late, which doesn't seem worth it.

🆗

@dougbu
Copy link
Member

dougbu commented Sep 21, 2020

What about your internal build will cause Microsoft.AspNetCore.App.Ref to be built❔

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 21, 2020

What about your internal build will cause Microsoft.AspNetCore.App.Ref to be built❔

I changed the 3.1.10 conditions to 3.1.9

@dougbu
Copy link
Member

dougbu commented Sep 21, 2020

In that case, the regular validation build will do the needful 😃

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 21, 2020

In that case, the regular validation build will do the needful 😃

The branch for the PR is versioned 3.1.9, but only builds the targeting pack when Patch is 10. In the internal build, I pushed another commit that changes the condition for the targeting pack so it builds when Patch is 9.

@dougbu
Copy link
Member

dougbu commented Sep 22, 2020

Lemme know when you've checked the artifacts from a build that should do the Right Thing:tm:

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Looks good though I'm curious why we need the ->Distinct() you added…

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 22, 2020

Looks good though I'm curious why we need the ->Distinct() you added…

I added it defensively, but the behavior is the same without it. Just removed it (same in the other PR).

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 22, 2020

Lemme know when you've checked the artifacts from a build that should do the Right Thing™️

Ref pack from the test build look good

@Pilchie Pilchie added the feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform label Sep 22, 2020
@dougbu
Copy link
Member

dougbu commented Sep 22, 2020

Alright. This is ready for 3.1.10 but needs to sit 'til the branches open for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants