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

Don't build a solution when -preprocess or -targets is passed on the command line #8588

Merged
merged 77 commits into from
Apr 7, 2023

Conversation

jrdodds
Copy link
Contributor

@jrdodds jrdodds commented Mar 22, 2023

Fixes #7697 (Partially)

Context

The -preprocess or -targets options are not supported for .sln files and, when these options are supplied, the .sln is built.

With this change the .sln will not be built.

Changes Made

Re-worked logic for handling the -preprocess or -targets options.

Testing

Tested on Windows 11 and macOS 12

Tested building and running unit tests and 'live' tested running MSBuild against a solution (.sln) file with the options.

Notes

Implementing -preprocess and -targets for solution files will be a separate PR.

jrdodds added 30 commits August 4, 2022 18:44
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good.

In case of running on solution - do we want to output a message indicating that operation is not currently supported? Silently not performing anything might seem confusing.

@jrdodds
Copy link
Contributor Author

jrdodds commented Mar 22, 2023

Looks good.

In case of running on solution - do we want to output a message indicating that operation is not currently supported? Silently not performing anything might seem confusing.

I agree that there should be a message. Just pushed a commit to add messages.

src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Show resolved Hide resolved
@danmoseley
Copy link
Member

Isn't /targets supported for a solution? It certainly used to be. There are many targets in the generated meta project?

@rainersigwald
Copy link
Member

Isn't /targets supported for a solution? It certainly used to be. There are many targets in the generated meta project?

Specifying -target:{} to build a specific target is still supported. What's not supported is the new -targets, which lists the targets available in the file to stdout (it was nontrivial to make it see past the solution metaproject so that part's just not done).

@rainersigwald rainersigwald added this to the VS 17.7 milestone Mar 28, 2023
{
success = PrintTargets(projectFile, toolsVersion, globalProperties, targetsWriter, projectCollection);
// TODO: Support /targets for solution files. https://github.com/dotnet/msbuild/issues/7697
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what the repo standard is (it's not my repo) but IMO todo comments and links to issues usually just make the code cluttered (often for years) when the same info could be found from the original PR or commit message.

Eg in this case I'd a simply comment something like "// /targets could be supported for solution files in future"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the TODO comments. I'm actively working on the issue to support /targets and /preprocess for solution files and this is intended to be an interim change. An earlier review requested adding the links. Regardless I have no issue with changing the comments if that's the standard and/or the consensus. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer to have the comment in the file, as it is now, as long as the link is to a permalink (we still have some that are like "regression test for #1283456" and I have no idea even what database that was in . . .)

@rainersigwald rainersigwald merged commit 1af12df into dotnet:main Apr 7, 2023
@rainersigwald
Copy link
Member

Thanks @jrdodds!

@jrdodds jrdodds deleted the Solution-Targets-Dont-Build branch April 7, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-targets command-line option doesn't work with solution file
5 participants