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

Add hook for importing .props file after .NET SDK props #45151

Open
wants to merge 1 commit into
base: release/9.0.2xx
Choose a base branch
from

Conversation

dsplaisted
Copy link
Member

Add a hook for a .props import after the .NET SDK props, which is useful for modifying the default item globs.

@Sergio0694
Copy link
Contributor

Seems like this would also cover part of ##1696? The "after SDK .targets" part I mean.

@dsplaisted
Copy link
Member Author

Seems like this would also cover part of ##1696? The "after SDK .targets" part I mean.

Thanks for pointing that out, this does address part of that request.

FYI @jeffkl @baronfel

Also, any preference on the property naming? I went with AfterMicrosoftNetSdkProps here, while the issue linked suggests CustomAfterMicrosoftNETSdkProps.

@Sergio0694
Copy link
Contributor

My two thoughts on naming, we should use "AfterMicrosoftNETSdkProps", for three reasons:

  • It's consistent with "BeforeMicrosoftNETSdkTargets" and "AfterMicrosoftNETSdkTargets" which already exist
  • Like those other two, these specifically refer to the .NET SDK .props/.targets, not the MSBuild ones, like eg. "CustomBeforeMicrosoftCommonProps" does. Not using "Custom" makes it easy to tell the two groups apart.
  • There is also a functional difference between all "Custom" ones and these .NET SDK ones. The "Custom" ones are imported behind an Exists(...) check, meaning they do not support appending. Instead you must store the original value, overwrite it, then manually import the original value from your custom .props/.targets, if there was one. Whereas with these .NET SDK ones, you can just append your .props/.targets to the property, since all items are imported at once. Leaving the "Custom" prefix just for the MSBuild ones with this legacy behavior would also further make it easier to tell them apart.

@jeffkl
Copy link
Contributor

jeffkl commented Nov 27, 2024

I personally don't have a strong opinion one way or the other. My preference in general is to use the Custom prefix just out of habit. But honestly in this case, just having the functionality and it be documented is good enough for me, the naming is probably fine as-is.

Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

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

Is there a good place to document this extensibility point?

@@ -170,4 +170,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<Import Project="$(MSBuildThisFileDirectory)../../Microsoft.NET.Sdk.WindowsDesktop/targets/Microsoft.NET.Sdk.WindowsDesktop.props" Condition="Exists('$(MSBuildThisFileDirectory)../../Microsoft.NET.Sdk.WindowsDesktop/targets/Microsoft.NET.Sdk.WindowsDesktop.props')" />
<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Windows.props" />

<!-- Hook for importing custom logic after .NET SDK props, for example for additional default item processing -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<!-- Hook for importing custom logic after .NET SDK props, for example for additional default item processing -->
<!--
Extensibility point for users to import build logic after all of the .NET SDKs .props are imported but before the project's contents are evaluated.
This can be useful for overriding default globs or properties that the SDK sets.
-->

@Sergio0694
Copy link
Contributor

"Is there a good place to document this extensibility point?"

+1 to this. I could not find any existing docs clearly documenting all existing MSBuild/.NET SDK hooks 😅

@dsplaisted dsplaisted added the consider-for-documentation Issues where some of the information in the discussion may be appropriate to add to documentation label Nov 28, 2024
@dsplaisted
Copy link
Member Author

FYI @baronfel @marcpopMSFT, I added the consider-for-documentation tag

@dsplaisted
Copy link
Member Author

/azp run dotnet-sdk-public-ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Sergio0694
Copy link
Contributor

@dsplaisted I tested this with the internal VS preview with the UWP SDK changes, works great! 🙂

@dsplaisted
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dsplaisted
Copy link
Member Author

/azp run dotnet-sdk-public-ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marcpopMSFT
Copy link
Member

@dsplaisted is this ready to go? Failures just look like known timeouts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-ILLink consider-for-documentation Issues where some of the information in the discussion may be appropriate to add to documentation untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants