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

Cleanup vcpkg_configure_cmake.cmake #6792

Merged
merged 4 commits into from Jul 9, 2019
Merged

Conversation

YenForYang
Copy link
Contributor

Cleanup code.

  • Remove/fix possibly redundant call to vcpkg_find_acquire_program(), thus setting PATH twice
  • Fix accessing ENV{PROCESSOR_ARCHITECTURE}, which is Windows only
  • Use macro()s to improve readability.
  • Use variables to reduce redundant checks and increase clarity

Cleanup code.
* Remove/fix possibly redundant call to `vcpkg_find_acquire_program()`, thus setting `PATH` twice
* Fix accessing `ENV{PROCESSOR_ARCHITECTURE}`, which is Windows only
* Use `macro()`s to improve readability.
* Use variables to reduce redundant checks and increase clarity
@Rastaban
Copy link
Contributor

Rastaban commented Jun 7, 2019

Overall this looks like a good improvement to the readability of the file. Changes to this file do not yet trigger a full build in the PR test system when the build pipelines run so we will want to run some additional tests for verification.

@Rastaban Rastaban self-assigned this Jun 7, 2019
@Rastaban
Copy link
Contributor

Rastaban commented Jul 1, 2019

resolved merge conflicts

@Rastaban
Copy link
Contributor

Rastaban commented Jul 2, 2019

Running full rebuild test 6792c https://dev.azure.com/vcpkg/public/_build/results?buildId=4906

@Rastaban
Copy link
Contributor

Rastaban commented Jul 9, 2019

This looks good, the only test failures in the full rebuild were unrelated to this change (some ports can be flaky)

@Rastaban Rastaban merged commit e7aafb7 into microsoft:master Jul 9, 2019
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.

None yet

3 participants