-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implement Command-line option for displaying targets #5032
Implement Command-line option for displaying targets #5032
Conversation
- TODO do we need new MSB error code or edit existing message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! These changes look good to me with just a few minor issues.
@@ -1056,6 +1060,7 @@ string outputResultsCache | |||
ToolsetDefinitionLocations toolsetDefinitionLocations = ToolsetDefinitionLocations.Default; | |||
|
|||
bool preprocessOnly = preprocessWriter != null && !FileUtilities.IsSolutionFilename(projectFile); | |||
bool targetsOnly = targetsWriter != null && !FileUtilities.IsSolutionFilename(projectFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change loadProjectReadOnly (10 lines down) to be !preprocessOnly && !targetsOnly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem logical for me at first glance, because printing the targets should be a read-only operation.
Also, it looks like the setting will be ignored anyway (see this comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I was suggesting it should be read-only, but I might have gotten it flipped. @rainersigwald, do you know better? Otherwise, this looks good to me.
- it was missing from original test
I doubt this would actually be practically useful to anyone. Since MSBuild does not have any information hiding capabilities (you get ALL the hundreds of meaningless targets, not just the entry graph wide operations supported by the current mix of "SDKs"), and since MSBuild does not differentiate the graph wide entry targets (Build, Clean, Rebuild, Test, Restore, Pack) in any way from the implementation targets, this feature will just lead to information overload. It would interesting though, if we introduced graph wide entry targets as first class entities in the language. I'd be quite curious to learn of a practical usage to this. |
One possible use I thought of was if you know roughly what you need but don't know exactly what the target is called. This would make it easier to find the exact target. You can also try to add custom targets, and if they don't show up, you know you didn't add them properly. The original issue has 30 upvotes and has gotten some activity within the past 6 months, so although it's an old issue, I think it's worth taking this pr. |
One thing I worry about on this feature is it might give the implication of a target contract. Looking at the other commands provided as examples, The other is this is pretty trivial to get as a one-off. It seems surprising to me that this would be common enough to go into MSBuild proper. Again unless it's more of a let's see what's available to hook into which is my original concern. |
src/Build/Definition/Project.cs
Outdated
@@ -1417,6 +1417,11 @@ public void SaveLogicalProject(TextWriter writer) | |||
implementation.SaveLogicalProject(writer); | |||
} | |||
|
|||
public void PrintTargets(TextWriter writer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have the Project API only return targets, and not deal with the extra problem domain of printing object model things to streams.
I'm happy to see that the PR sparked a bit of conversation [also, a bit less happy about the merge conflict]. I'll update as soon as I have a bit of free time. About usability of the output: I also think that dumping 200 targets is not the most useful thing we could do. For now, I can consider this feature more like a debugging tool than quick help for the confused end-user. Also, would not recommend skipping targets based on naming convention. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The team talked about the possibility of sorting the results, but decided not to bother:
- The ideal sort order is by dependency order, but that's not statically knowable.
- If you just want alphabetical sort, it's easy to pipe to
sort
or similar.
src/MSBuild/XMake.cs
Outdated
} | ||
catch (Exception ex) | ||
{ | ||
var message = ResourceUtilities.FormatResourceStringStripCodeAndKeyword("TargetsCouldNotBePrinted", ex.Message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var message = ResourceUtilities.FormatResourceStringStripCodeAndKeyword("TargetsCouldNotBePrinted", ex.Message); | |
var message = ResourceUtilities.FormatResourceStringStripCodeAndKeyword("TargetsCouldNotBePrinted", ex); |
This preserves the call stack which can make fixing bugs that report this error MUCH easier.
src/MSBuild/XMake.cs
Outdated
{ | ||
Project project = projectCollection.LoadProject(projectFile, globalProperties, toolsVersion); | ||
|
||
targetsWriter.WriteLine(string.Join(Environment.NewLine, project.Targets.Keys)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you call this in a loop?
targetsWriter.WriteLine(string.Join(Environment.NewLine, project.Targets.Keys)); | |
foreach (string target in project.Targets.Keys) | |
{ | |
targetsWriter.WriteLine(target); | |
} |
That way we don't have to allocate a big string? (it's not super critical here, but it's good practice)
…line # Conflicts: # src/MSBuild/Resources/Strings.resx # src/MSBuild/Resources/xlf/Strings.cs.xlf # src/MSBuild/Resources/xlf/Strings.de.xlf # src/MSBuild/Resources/xlf/Strings.en.xlf # src/MSBuild/Resources/xlf/Strings.es.xlf # src/MSBuild/Resources/xlf/Strings.fr.xlf # src/MSBuild/Resources/xlf/Strings.it.xlf # src/MSBuild/Resources/xlf/Strings.ja.xlf # src/MSBuild/Resources/xlf/Strings.ko.xlf # src/MSBuild/Resources/xlf/Strings.pl.xlf # src/MSBuild/Resources/xlf/Strings.pt-BR.xlf # src/MSBuild/Resources/xlf/Strings.ru.xlf # src/MSBuild/Resources/xlf/Strings.tr.xlf # src/MSBuild/Resources/xlf/Strings.zh-Hans.xlf # src/MSBuild/Resources/xlf/Strings.zh-Hant.xlf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks!
Thanks @szaliszali! This should go out with Visual Studio 16.6 preview 1 (at some point in the future). |
Is it available in VS 2019? |
It should be. What exact version of VS 2019 are you using? 16.11? |
VS 2019 16.9.25 build. |
After the update, it is working. I am wondering if there is something for solution file (.sln) as well. |
This feature request was lying around for a few years, I'm submitting a simple implementation for review and further discussion. As @AndyGerlicher kindly pointed out the already existing
preprocess
option, it seemed logical to take the same approach for this one.Sample output for a C# project:
Fixes #33