FYI @KirillOsenkov

Looks great. I guess adding more special-case handling of the restore target should boost the priority of #9553.

100% agree on we should add mechanism flagging that issue.
Though it seems it won't be able to benefit from this change - as the Restore in this case is 'marked' only if it's the only executed target of the build and explicitly specified. If multiple targets are explicitly given (e.g. /t:'Build;Execute'), or if the Restore is added in the script (e.g. InitialTragets="Restore" or <BuildDependsOn>$(BuildDependsOn);Restore</BuildDependsOn>) then it's not marked.
So we'll probably need to infer the situation from the build execution (e.g. TargetStartedEvent args with the Restore or Pack target and as well as with the Build target are encountered with the same ProjectContextId)

I wonder if in this case we could get away with adding a high-importance message right to XMake.cs (as opposed to building an analyzer).

I wonder if in this case we could get away with adding a high-importance message right to XMake.cs (as opposed to building an analyzer).

I support that!

It wouldn't catch the case of restore run requested via dependencies definition - but I'd hope it's rather esoteric. And even if it wouldn't be esoteric - check in XMake is cheap and we can have it quickly... - do you want to shoot a PR? :-)

Wait, I don't think I understand what the message is for? Is it "you're running Restore and something else in the same invocation"?

Yes, it would basically warn against /t:Restore;SomethingElse. Because it looks like it's broken enough to maybe warrant this.

I fear that's the same kind of "breaking change" we've had to back out before. I think the analyzer-related "have a way to opt into new warnings" would have to be in place first.

Question for everyone:

Should this pull request also set EmbedProjectAssetsFile to false or should I update the .NET SDK to default EmbedProjectAssetsFile to false when MSBuildIsRestoring is true?

https://github.com/dotnet/sdk/blob/7b872bf735f7d86b6c20281ab9f7a55d9c712a91/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets#L69

I'd leave that definition in the SDK I think.

Hi @jeffkl,

These changes backfired in an unusual way for the tests in sdk (e.g. dotnet/sdk#39093)
You can see the attached binlogs here
sdk_test_binlogs.zip

In the test, publish and restore are run on the same project separately, and on restore execution PublishTrimmed switch is passed in a special way
https://github.com/dotnet/sdk/blob/0a2a432e980a143c6f7a9827d3ce015b39564427/test/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs#L2252 (hopefully @MiYanni has some story behind this, please let us know!)
In fact it writes a new property to *.g.targets and expects from msbuild to react to it!

Since restore happens after publish, project is considered as up to date and this stage is skipped:

As a possible strategies we can suggest:

1. Make changes on MSBuild side and check *.g.targets for up to date state;
2. Revert the changes if it can be considered as a real world scenario;
3. Probably something can be adapted on the Nuget's side (?).

Please share you opinion on that!
Thank you.

also adding @Forgind for his insights

If my understanding is correct, we should update the tests to account for the new behavior.

I've filed an issue here and added some details:
dotnet/sdk#39266

jeffkl and I investigated this yesterday in the SDK insertion PR and found the same root cause as YuliiaKovalova. We also decided it's probably best to just update the tests. We're still talking a bit (with dsplaisted) about how big of a breaking change it likely is and what we should do about it. Thanks for the parallel investigation!

If my understanding is correct, we should update the tests to account for the new behavior.

I've filed an issue here and added some details: dotnet/sdk#39266

Even switching to inline argument will help!

But I though it was intentionally passed this way for checking some specific scenario which I am not aware of.

@YuliiaKovalova

I was trying to understand how I'm related to this since I don't even know what this is. Took me a second as I looked at the blame on the file, and I'm not in the blame. However, I'm in the commit history for the file because I moved every test in the repo in this PR. So, I'd just recommend looking at the blame history over the commit history since it usually gives you a more accurate gauge of the people involved.