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

How to check if the current invocation is running publish? #26324

Open
agocke opened this issue Jun 28, 2022 · 19 comments
Open

How to check if the current invocation is running publish? #26324

agocke opened this issue Jun 28, 2022 · 19 comments
Milestone

Comments

@agocke
Copy link
Member

agocke commented Jun 28, 2022

Title basically says it all -- but in certain cases I'd like to know in the project file whether we're doing a build or a publish. In particular, I might want a non-self-contained build by default, but always want a self-contained publish.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels Jun 28, 2022
@agocke
Copy link
Member Author

agocke commented Jun 28, 2022

@dsplaisted
Copy link
Member

There isn't a way to tell what targets MSBuild will run (before it actually runs them), so there's not a good way to do this.

On the other hand, we would like to be able to have different defaults and behavior between build and publish. The first step toward that just went in with #25991.

With that change, you could check the _IsPublishing property to see if the build was run via dotnet publish. However, the property would not be set if you ran dotnet build /t:Publish or msbuild /t:Publish.

@marcpopMSFT
Copy link
Member

@dsplaisted are there plans (or even capability) to set ispublishing when using the /t flag? Not sure what this issue tracks with 25991 merged now.

@dsplaisted
Copy link
Member

No concrete plans I think. We could detect and handle something like /t:Publish in the CLI, but that wouldn't handle cases like /t:TargetThatDependsOnPublish. FYI @baronfel

@baronfel
Copy link
Member

yeah, this is a sort of general problem we're hitting with all of these 'use publish to push better packaging practices'-style features - the gulf between dotnet X and dotnet msbuild /t:X grows over time. As I understand it @rainersigwald has been reluctant in the past (for good reason!) to add something like @(MSBuildDesiredTarget) as a known property, so we don't really have a good way to get out of this hole in a general fashion.

@agocke
Copy link
Member Author

agocke commented Jun 29, 2022

I think the _IsPublishing property is basically what I needed. I'm not sure much more than that would be necessary for my scenarios. I would prefer it to be "public", but otherwise it sounds good to me.

@agocke
Copy link
Member Author

agocke commented Jun 29, 2022

Aside -- could we just set IsPublishing in the Publish target? Is there some reason I'm missing why this isn't sufficient?

@dsplaisted
Copy link
Member

Aside -- could we just set IsPublishing in the Publish target? Is there some reason I'm missing why this isn't sufficient?

In that case it wouldn't be set during MSBuild evaluation or in targets that run before publish. So I think in most cases it would be set way later than you would need to check it.

@agocke
Copy link
Member Author

agocke commented Jun 29, 2022

I see, in that case I could also set it to true in the Publish target in case of /t:Publish?

@nagilson
Copy link
Member

nagilson commented Jul 6, 2022

I see, in that case I could also set it to true in the Publish target in case of /t:Publish?

No, see this discussion here: it (referring to _IsPublishing) needs to be defined as property. I don't know of any way to tell what the target is early enough for MSBuild evaluation to work properly. I hope this is helpful. #25991 (comment) @agocke

@gkulin gkulin added needs team triage Requires a full team discussion and removed untriaged Request triage from a team member labels Jul 13, 2022
@marcpopMSFT marcpopMSFT removed the needs team triage Requires a full team discussion label Jul 13, 2022
@marcpopMSFT marcpopMSFT added the good first issue Issues that would be a good fit for someone new to the repository. Narrow in scope, well-defined. label Jul 13, 2022
@marcpopMSFT marcpopMSFT added this to the Backlog milestone Jul 13, 2022
@marcpopMSFT
Copy link
Member

One suggestion is to capture anything specified through -t: on the commandline into a global property that's a semicolon separated list thing dotnet build -t:Publish -t:Test sets $(UserTargets)=Publish;Tests.

Noah took care of dotnet publish and dotnet pack setting a global property of _IsPublishing

@xoofx
Copy link
Member

xoofx commented Dec 19, 2022

yeah, this is a sort of general problem we're hitting with all of these 'use publish to push better packaging practices'-style features - the gulf between dotnet X and dotnet msbuild /t:X grows over time. As I understand it @rainersigwald has been reluctant in the past (for good reason!) to add something like @(MSBuildDesiredTarget) as a known property, so we don't really have a good way to get out of this hole in a general fashion.

Hey, I stumbled on this issue, as I wanted to set PublishAot by default to true in a target file, but only if we are in publish mode, so now I'm using _IsPublishing and it is okish.

But I'm wondering why it would not be desirable to have MSBuildDesiredTarget (or a property $MSBuildDesiredTarget_$(ActualTarget) that would e.g set $MSBuildDesiredTarget_Build or $MSBuildDesiredTarget_Publish to true if they are targeted)? It does seem that we have some usecases here that are indicating that it would be more convenient to have it built-in than to hack around it specifically for e.g Publish?

@nagilson
Copy link
Member

@xoofx @/rainersigwald can provide a better explanation than me but I will give it an attempt.

It is 'impossible' to know for sure which target is going to run early enough for MSBuild itself to take action. One reason is that people can implement their own custom targets that call publish or build later, so from the beginning of evaluation we cannot check the MSBuild target stack to see if Publish is in the target-stack.

Another reason is that properties like PublishRelease, which change the Configuration, or PublishAot, which affect and rely on early build components like RuntimeIdentifier, and Optimize, need to be set very early. MSBuild is designed on the core concept that a target will run exactly one time for a given set of global properties. (E.G. Even if you build for two different TFM, MSBuild will only take the overhead of running CSC / Core Compile only once.) In a world where there is a "IsTargetMode" that tries to change things which were already evaluated, e.g. the bin/CONFIGURATION path, that promise would be broken.

We would love to have the ability to do so, and in fact I directed a meeting with others to discuss if we could, the above is the end result of that meeting. VS and Dotnet are different because when you invoke dotnet publish you explicitly know before evaluation that you're doing a publish, same when you click the button in VS. (Though _IsPublishing is not set in VS because I could not convince them it was worth accepting the risk of adding such a thing. 😉 )

BTW, ❤️ Unity

@xoofx
Copy link
Member

xoofx commented Dec 19, 2022

Thanks for the explanation @nagilson, I appreciate the details!

It is 'impossible' to know for sure which target is going to run early enough for MSBuild itself to take action. One reason is that people can implement their own custom targets that call publish or build later, so from the beginning of evaluation we cannot check the MSBuild target stack to see if Publish is in the target-stack.

Indeed, I understand that it's safer to plug it in the dotnet command. If I recall, it's not the only property that some commands are already setting or that the interaction with MSBuild is tweaked from there (e.g build => restore + build in separate steps)

Hope that _IsPublishing will stay stable, always a bit worried when I see a _ in front of a property, as I have always assumed that they are considered as "private" MSBuild properties in the past that we should not rely on! 😅

BTW, ❤️ Unity

Same, will love it even more once we have migrated to .NET 7/8 + MSBuild! 😄

@MichalStrehovsky
Copy link
Member

Did we consider doing a breaking change and erroring out in the Publish target if _IsPublishing is not set?

The problem is that we're getting more and more build customizations conditioned under _IsPublishing. External packages have similar needs (most recently microsoft/CsWinRT#1547). We're now ending up in a situation where dotnet build /t:Publish will do very different things from dotnet publish. It might not be obvious what things are different, why, and how to fix it. Requiring to set a property before one is allowed to run the target could be one way to address that. This assumes we'd also make the property public so that people who want to run publish target can set it using a supported mechanism.

@nagilson
Copy link
Member

nagilson commented Mar 21, 2024

@MichalStrehovsky We never considered that. I can see the logic behind it, though it would break a large number of customers and probably cause frustration. That being said, it is an interesting idea to solve this issue that you mention regarding _IsPublishing.

@MichalStrehovsky
Copy link
Member

https://github.com/search?q=_IsPublishing+language%3AXML&type=code&l=XML - looks like many third parties depend on _IsPublishing. All of those uses will not do the intended thing when someone runs /t:Publish instead of the real publish.

I guess the question is at what point will /t:Publish without /p:_IsPublishing=true become more breaking than just erroring out with a prescriptive/self-serviceable message.

We're starting to get blocked internally on things because /t:Publish without /p:_IsPublishing=true is a thing: #37872 (comment)

@nagilson
Copy link
Member

Wow. _IsPublishing has only been around for about a year. As the creator of this thing, I can tell you we didn't expect that given the _.

Anyway, I think your point is interesting. Since @rainersigwald owns MS Build I think he would have to have a say on what we'd do here, as well as our PMs. But its something we could consider.

@nagilson nagilson removed the good first issue Issues that would be a good fit for someone new to the repository. Narrow in scope, well-defined. label May 2, 2024
@nagilson
Copy link
Member

nagilson commented May 2, 2024

Definitely not a 'good first issue'

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

No branches or pull requests

8 participants