-
Notifications
You must be signed in to change notification settings - Fork 45
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
Address review feedback from PR #485 #486
Conversation
CC: @KyleFromKitware This does not need a formal review. It just addresses post-merge review comments from PR #485. I am going to go ahead and merge this so that I can snapshot TriBITS into a new Trilinos branch and get a Trilinos PR posted for that. |
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.
CC: @KyleFromKitware
This does not need a formal review. It just addresses post-merge review comments from PR #485.
I am going to go ahead and merge this so that I can snapshot TriBITS into a new Trilinos branch and get a Trilinos PR posted for that.
In this case, there are no spaces in the --pretty=format:<value> argument so we don't need the quotes and therefore don't need to suffer the complexity of removing them after the fact. But note the case I copied and pasted this from in tribits_generate_single_repo_version_string() still requires them because that format spec has spaces.
This was caught in review by @KyleFromKitware. Turns out that TriBITS was using find_program(Git) everywhere else already.
It was pointed out by @KyleFromKitware in the review of PR #485 that calling cmake_policy() in a function will change a policy in the calling scope. NOTE: The function() command does not create a new policy scope but does create a new variable scope. Just the opposite, include() creates a new policy scope but not a new variable scope.
With newer CMake version, you can just quote the entire argument: "--pretty=format:<fmnt>" where <fmnt> may have spaces and everything seems to work correctly.
4bd215a
to
e82c454
Compare
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.
Approving rebased commits
This PR addresses post-merge review comments from PR #485 from @KyleFromKitware.
See the commits and commit logs for more details.