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

Change Version switch output to finish with a newline #9485

Merged
merged 4 commits into from
Dec 13, 2023

Conversation

jrdodds
Copy link
Contributor

@jrdodds jrdodds commented Dec 1, 2023

Fixes #9482

Context

The -version switch doesn't terminate its output with a newline which some shells don't like.

Changes Made

  • Added a unit test.
  • Changed ShowVersion().

Testing

Tested on Windows 10 and macOS 14.
Tested by running the full unit test suite and by manually running the -version switch under cmd (Windows), pwsh (Windows and macOS), and zsh (macOS).

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

This could, conceivably, break someone who has a script that's calling msbuild -nologo -version | script_that_cant_handle_newline, right? I hate that I'm so conservative but I think I want it behind a changewave.

@jrdodds
Copy link
Contributor Author

jrdodds commented Dec 1, 2023

Yes, that's conceivable. And for someone who has written code like that and hits this change, the defect would be mysterious.

@jrdodds
Copy link
Contributor Author

jrdodds commented Dec 1, 2023

I think I should change the VersionSwitch unit test from a Theory to a Fact and just test one variant of the version switch. Covering multiple variants is somewhat redundant with CommandLineSwitchesTests.VersionSwitchIdentificationTests. (I started with the Help unit test as a model.)

@rainersigwald
Copy link
Member

That sounds fine to me.

@jrdodds
Copy link
Contributor Author

jrdodds commented Dec 4, 2023

Should this be in a change wave and should it be change wave 17.10?

@rainersigwald
Copy link
Member

Yes and yes, please.

@rainersigwald rainersigwald changed the base branch from main to vs17.9 December 6, 2023 23:05
Copy link
Contributor

@f-alizada f-alizada left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!
I left one comment with question about the verification of two scenarios in one test, overall looks good :)

src/MSBuild.UnitTests/XMake_Tests.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@f-alizada f-alizada left a comment

Choose a reason for hiding this comment

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

Thank you for the updates! Left one comment regarding the base branch to be merged

eng/Versions.props Outdated Show resolved Hide resolved
eng/Version.Details.xml Outdated Show resolved Hide resolved
@f-alizada f-alizada merged commit 1ac1bff into dotnet:vs17.9 Dec 13, 2023
8 checks passed
@jrdodds jrdodds deleted the VersionMessage branch December 15, 2023 13:50
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.

[Bug]: -version message doesn't have a line ending
4 participants