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

[7.0] Fix intellisense xml file selection from 'net' or 'dotnet-plat-ext' #82554

Merged
merged 4 commits into from
Mar 9, 2023

Conversation

carlossanlop
Copy link
Member

Addresses the 7.0 bug of #82214

There are two places in 7.0 where the intellisense xml files are analyzed and/or copied.

  • In packaging.targets, we decide if we want to include in the assembly package either the built xml or the one that comes from the internal intellisense nupkg. If we decide to use the latter, we were missing logic that would find the right xml file in either the 'net' or the 'dotnet-plat-ext' folder of the extracted nupkg. Instead, we were only looking inside 'net', and assemblies like System.IO.Ports were not getting anything included in its nupkg.

  • In docs.targets, we were only copying to artifacts/bin/docs the files that were found in the 'net' folder of the internal intellisense nupkg. We need to also copy the files from 'dotnet-plat-ext', so that assemblies like System.IO.Ports get the right documentation shipped.

cc @krwq

@ghost
Copy link

ghost commented Feb 23, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Addresses the 7.0 bug of #82214

There are two places in 7.0 where the intellisense xml files are analyzed and/or copied.

  • In packaging.targets, we decide if we want to include in the assembly package either the built xml or the one that comes from the internal intellisense nupkg. If we decide to use the latter, we were missing logic that would find the right xml file in either the 'net' or the 'dotnet-plat-ext' folder of the extracted nupkg. Instead, we were only looking inside 'net', and assemblies like System.IO.Ports were not getting anything included in its nupkg.

  • In docs.targets, we were only copying to artifacts/bin/docs the files that were found in the 'net' folder of the internal intellisense nupkg. We need to also copy the files from 'dotnet-plat-ext', so that assemblies like System.IO.Ports get the right documentation shipped.

cc @krwq

Author: carlossanlop
Assignees: carlossanlop
Labels:

area-Infrastructure-libraries

Milestone: 7.0.x

eng/packaging.targets Show resolved Hide resolved
eng/restore/docs.targets Show resolved Hide resolved
@carlossanlop
Copy link
Member Author

All CI failures have been logged above. They're unrelated.

@ViktorHofer if we want to properly fix the issue, we would have to enable GeneratePackageOnBuild and bump the ServicingVersion of all affected nuget packages.

I'd feel more comfortable doing this in a separate PR. Any suggestions/concerns about this? Same question would apply to the 6.0 version of this PR.

@ViktorHofer
Copy link
Member

We would need to re-ship quite a lot of packages so let's definitely not do this in this PR. IMO we should discuss this in one of the next Tactics meetings to get pre-approval before we even attempt to do the work.

@carlossanlop
Copy link
Member Author

We would need to re-ship quite a lot of packages so let's definitely not do this in this PR. IMO we should discuss this in one of the next Tactics meetings to get pre-approval before we even attempt to do the work.

Let's at least ship a new version of System.IO.Ports, since the issue started with that one. We can add the rest to the same PR depending on what Tactics says.

@carlossanlop
Copy link
Member Author

Failures unrelated.

@carlossanlop carlossanlop merged commit 323bdf3 into dotnet:release/7.0 Mar 9, 2023
@carlossanlop carlossanlop deleted the IntellisenseXml7 branch March 9, 2023 01:25
@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants