Change execution order conditions for intellisense swapping and copying msbuild targets. #83117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ng msbuild targets.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsFixes Issue #82191 The current conditions used to decide when to execute the intellisense related targets need to be changed to avoid overbuilding.
|
carlossanlop
added
the
documentation
|
These changes look good. @carlossanlop this doesn't fix #82191 as we still depend on TargetFramework agnostic builds, even if there's a closer OS specific target framework. This change fixes #79030 or #82214 (whichever of these two issues apply to main). |
|
I updated the description. |
|
Just to clarify:
Is it your preference that we get that issue fixed in this PR? |
|
Fixing that isn't a priority right now and it's yet unclear how to best approach that issue. Let's keep this scoped on fixing the sequencing points. |
|
@carlossanlop I think we want to get this in before Preview 3 snap (which I believe is next Friday?). Were you able to test the changes and see if our packages now contain the expected intellisense file? |
|
The AllConfigurations build is failing: I was able to repro after rebasing my branch to the latest bits. Not sure why I didn't see it before rebasing: |
|
@carlossanlop I pushed to your branch, hope that's OK. The problem was that the While at it, I also cleaned-up code in the file. |
carlossanlop
commented
Mar 15, 2023
| <PropertyGroup> | ||
| <NoWarn Condition="'$(UseIntellisensePackageDocXmlFile)' == 'true'">$(NoWarn);1591</NoWarn> | ||
| <PropertyGroup Condition="'$(UseIntellisensePackageDocXmlFile)' == 'true'"> | ||
| <NoWarn>$(NoWarn);1591</NoWarn> |
There was a problem hiding this comment.
Why do we need 1591 here?
carlossanlop
commented
Mar 15, 2023
| <IntellisensePackageXmlRootFolder>$([MSBuild]::NormalizeDirectory('$(NuGetPackageRoot)', 'microsoft.private.intellisense', '$(MicrosoftPrivateIntellisenseVersion)', 'IntellisenseFiles'))</IntellisensePackageXmlRootFolder> | ||
| <IntellisensePackageXmlFilePathFromNetFolder>$([MSBuild]::NormalizePath('$(IntellisensePackageXmlRootFolder)', 'net', '1033', '$(AssemblyName).xml'))</IntellisensePackageXmlFilePathFromNetFolder> | ||
| <IntellisensePackageXmlFilePathFromDotNetPlatExtFolder>$([MSBuild]::NormalizePath('$(IntellisensePackageXmlRootFolder)', 'dotnet-plat-ext', '1033', '$(AssemblyName).xml'))</IntellisensePackageXmlFilePathFromDotNetPlatExtFolder> | ||
| <IntellisensePackageXmlFilePath Condition="'$(UseIntellisensePackageDocXmlFile)' == 'true' and Exists($(IntellisensePackageXmlFilePathFromNetFolder))">$(IntellisensePackageXmlFilePathFromNetFolder)</IntellisensePackageXmlFilePath> | ||
| <IntellisensePackageXmlFilePath Condition="'$(IntellisensePackageXmlFilePath)' == '' and '$(UseIntellisensePackageDocXmlFile)' == 'true' and Exists($(IntellisensePackageXmlFilePathFromDotNetPlatExtFolder))">$(IntellisensePackageXmlFilePathFromDotNetPlatExtFolder)</IntellisensePackageXmlFilePath> | ||
| <IntellisensePackageXmlFilePath Condition="'$(IntellisensePackageXmlFilePath)' == '' and Exists($(IntellisensePackageXmlFilePathFromNetFolder))">$(IntellisensePackageXmlFilePathFromNetFolder)</IntellisensePackageXmlFilePath> |
There was a problem hiding this comment.
OK cool. I see it makes more sense to check its own value.
There was a problem hiding this comment.
CI is green now, nice!
|
Testing locally now... |
|
CI is green and tests below look good. The only results that always failed were the xml files generated under the The files that matter are the ones under I think this is good to merge, @ViktorHofer. I'll hit the button. TSASOT = Triple slash as source of truth. TestsIndividual commandsSystem.Formats.Cbor - TSASOT, OOB.
System.Numerics.Vectors - TSASOT, partial source facade.
System.Formats.Tar - MS Docs.
System.IO.Compression.Brotli - MS Docs, PNSE.
System.IO.Ports - OOB, PNSE, partial source facade, MS Docs.
Full buildPassed: |
|
Ah, @ViktorHofer, I don't yet have a sign-off. Please do the honors and merge this PR. |
ViktorHofer
approved these changes
Mar 16, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #82214
The current conditions used to decide when to execute the intellisense related targets need to be changed to ensure OOB packages get their correct intellisense files included in their packages.