Skip to content

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 :)

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

@f-alizada f-alizada merged commit 1ac1bff into dotnet:vs17.9 Dec 13, 2023
@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