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

Create dummy AfterSdkPublish target Fixes #26710 #36850

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Nov 14, 2023

Fixes #26710

After discussions with @baronfel and @vijayrkn, it sounds like the cleanest solution for customers who want an "AfterPublish" target in non-web projects is to introduce a new dummy "AfterSdkPublish" target in the base SDK. There appear to be no references to such a target on GitHub, through an online search, or in the DevDiv repo on AzDO, so this is likely safe and hopefully helpful.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels Nov 14, 2023
@baronfel baronfel requested a review from a team November 28, 2023 21:25
@baronfel
Copy link
Member

When we merge this @Forgind, can you work with me and @gewarren to find the best place to document this in the learn.microsoft.com docs?

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

It would be good to have some tests for this.

@marcpopMSFT marcpopMSFT merged commit 10d1f1c into dotnet:release/8.0.2xx Dec 5, 2023
@oskarb
Copy link

oskarb commented Feb 23, 2025

I have a strong suspicion that this change does not actually work for all cases.

The unit test seems to only cover the non-web case (when _IsAspNetCoreProject would be false or unset).

For the web case... it looks to me like the earliest time _IsAspNetCoreProject is set to true is inside the _InitProjectCapabilityProperties target:

<Target Name="_InitProjectCapabilityProperties">
<PropertyGroup>
<_IsAspNetCoreProject Condition="%(ProjectCapability.Identity) == 'AspNetCore'">true</_IsAspNetCoreProject>
</PropertyGroup>
</Target>

In other words, isn't _IsAspNetCoreProject always unset at the point where this PR evaluates it first thing in Sdk.props? Thereby making _AfterSdkPublishDependsOn always contain Publish, and thus making this PR fail to resolve its goal (i.e. to provide a single hook target that works for both web and non-web projects.)

@oskarb
Copy link

oskarb commented Feb 23, 2025

Should it be based on UsingMicrosoftNETSdkWeb instead of _IsAspNetCoreProject?

<UsingMicrosoftNETSdkWeb>true</UsingMicrosoftNETSdkWeb>

Forgind added a commit that referenced this pull request Feb 28, 2025
This is based on feedback:
#36850 (comment)
#26710 (comment)

It seems that the property used for this for web-based projects is typically set in a target, which means it isn't available at evaulation time. This changes the property used to determine if a project is web-based to one set by importing the web sdk.

Although I think it should not be used in almost any case, it also adds the option of specifying that the target should run after a specified target. If a new project type comes up that has an AfterAfterPublish, for instance, and we want to run this after that, this makes it easy to support that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-NetSDK untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants