Skip to content

Re-enable test that was blocking codeflow#41135

Merged
agocke merged 1 commit intodotnet:release/8.0.1xxfrom
sbomer:reenableTest
May 24, 2024
Merged

Re-enable test that was blocking codeflow#41135
agocke merged 1 commit intodotnet:release/8.0.1xxfrom
sbomer:reenableTest

Conversation

@sbomer
Copy link
Copy Markdown
Member

@sbomer sbomer commented May 23, 2024

This test is running GetValuesCommand with PublishTrimmed set to true in the generated
<project>.WriteValuesToFile.g.targets. In the failure case, ImportProjectExtensionProps is false (due to
https://github.com/dotnet/msbuild/blob/147ecadd19ae031d5a511ad55908cff9bcdc17c5/src/Tasks/Microsoft.Common.props#L68 imported from full framework MSBuild props) during restore, so this file isn't imported, we don't set PublishTrimmed, Microsoft.NET.ILLink.Tasks isn't restored, and then ILLinkTargetsPath never gets set.

I was wondering why this wasn't failing in main on .NET Core, and found that the GetValuesCommand behavior was fixed as part of #39088 to not rely on ImportProjectExtensionProps. So an alternative fix would be to backport the GetValuesCommand fix.

This change is just a simpler fix to set PublishTrimmed in the project file for this testcase.

fixes #40882

This test is running `GetValuesCommand` with `PublishTrimmed` set
to `true` in the generated
`<project>.WriteValuesToFile.g.targets`. In the failure case,
`ImportProjectExtensionProps` is false (due to
https://github.com/dotnet/msbuild/blob/147ecadd19ae031d5a511ad55908cff9bcdc17c5/src/Tasks/Microsoft.Common.props#L68
imported from full framework MSBuild props) during restore, so
this file isn't imported, we don't set `PublishTrimmed`,
Microsoft.NET.ILLink.Tasks isn't restored, and then
`ILLinkTargetsPath` never gets set.

I was wondering why this wasn't failing in main on .NET Core, and
found that the `GetValuesCommand` behavior was fixed as part of
dotnet#39088 to not rely on
`ImportProjectExtensionProps`. So an alternative fix would be to backport the GetValuesCommand fix.

This change is just a simpler fix to set `PublishTrimmed` in the
project file for this testcase.
@sbomer sbomer requested a review from dsplaisted May 23, 2024 23:44
@ghost ghost added Area-ILLink untriaged Request triage from a team member labels May 23, 2024
@sbomer sbomer requested a review from nagilson May 23, 2024 23:44
Copy link
Copy Markdown
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

Thank you for following up with us, your investigation looks solid to me! We should probably add backporting that to our agenda at some point but 8.0.1 is kinda on lockdown atm.

@marcpopMSFT Do we need to take a test change thru servicing for 8 0 100 right now?

@marcpopMSFT
Copy link
Copy Markdown
Member

@marcpopMSFT Do we need to take a test change thru servicing for 8 0 100 right now?
If you're asking if we need approval, test changes don't require approval. We are locked down for 8 till July though.

Copy link
Copy Markdown
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-ILLink untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants