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

[vcpkg-cmake] Fix the application of VCPKG_CMAKE_CONFIGURE_OPTIONS with more than one option #25123

Merged

Conversation

TheJCAB
Copy link
Member

@TheJCAB TheJCAB commented Jun 7, 2022

Describe the pull request

VCPKG_CMAKE_CONFIGURE_OPTIONS (plural) is meant to be a list of options to be added to a cmake invocation.

But it was being applied quoted, which disallowed it from containing multiple options.

  • If it was initialized with a list, cmake would get the options ";"-separated.
  • If it was initialized with a blank-separated string, cmake would get the entire string quoted as a single argument.

The fix is simple: don't (ever) quote arguments to list(APPEND) unless they are really meant to be a single element to append to the list.

  • What does your PR fix?

Fixes an issue I encountered when adding a custom triplet that uses a toolchain file.

I see issues #17750 and #23741 (and 23852 more indirectly) all contain reports of multiple options working correctly. Apparently PR #23259 inadvertently broke it by substituting list() with vcpkg_list().

vcpkg_list() top comments say "A replacement for CMake's list() function, which correctly handles elements
with internal semicolons (in other words, escaped semicolons)", but fail to explain what's the nature of the issue, and the code is not trivial to follow, so I'm guiding myself from empirical observation. Explanations and insights are welcome. In the meantime, removing the quotes here works around the issue.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

N/A

Yes (it's just basically a typo fix).

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

N/A

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for vcpkg-cmake have changed but the version was not updated
version: 2022-05-10
old SHA: e8db2f70aa8b584aac932fcff65d91bf52d57731
new SHA: d7d00bd5179b04630039999dad32abb3f4185264
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@TheJCAB
Copy link
Member Author

TheJCAB commented Jun 7, 2022

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions
When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

I don't understand what the bot is asking for, here. This seems unfriendly to the uninitiated. I'm just fixing basically a typo-like issue :-).

@TheJCAB
Copy link
Member Author

TheJCAB commented Jun 7, 2022

I don't understand what the bot is asking for, here.

@walbourn assisted offline. Appreciated.

@TheJCAB
Copy link
Member Author

TheJCAB commented Jun 7, 2022

And after modifying the version, now it all has a merge conflict with another PR that beat me by a matter of minutes.

I am sending this PR from a fork (github way of doing things?). I don't know how to pull the new master branch into my fork so I can re-resolve the conflict (git fetch does nothing), and I don't know what the correct resolution is now:

  • Just increase version-date.
  • Just increase port-version.
  • Increase both.

More assistance appreciated.

…ne option

VCPKG_CMAKE_CONFIGURE_OPTIONS (plural) is meant to be a list of options to be added to a cmake invocation.

But it was being applied quoted, which disallowed it from containing multiple options.
- If it was initialized with a list, cmake would get the options ";"-separated.
- If it was initialized with a blank-separated string, cmake would get the entire astring quoted as a single argument.

The fix is simple: don't (ever) quote arguments to list(APPEND) unless they are really meant to be a single element to append to the list.
@TheJCAB TheJCAB force-pushed the user/jcab/Fix_VCPKG_CMAKE_CONFIGURE_OPTIONS branch from c239b77 to 49ce487 Compare June 8, 2022 01:32
@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly and removed category:port-bug The issue is with a library, which is something the port should already support labels Jun 8, 2022
@JackBoosY
Copy link
Contributor

I think this changes is correct, @Neumann-A what do you think about?

@dg0yt
Copy link
Contributor

dg0yt commented Jun 8, 2022

vcpkg_list() top comments say "A replacement for CMake's list() function, which correctly handles elements
with internal semicolons (in other words, escaped semicolons)", but fail to explain what's the nature of the issue, and the code is not trivial to follow, so I'm guiding myself from empirical observation. Explanations and insights are welcome.

vcpkg_list() is one of the few functions which has a unit test. This can give insight in some of the quirks resolved by the function:
https://github.com/microsoft/vcpkg/blob/master/scripts/test_ports/unit-test-cmake/test-vcpkg_list.cmake

In the meantime, removing the quotes here works around the issue.

I think this is the right change here.

@JackBoosY
Copy link
Contributor

There is one thing that need to be noticed is that this issue may also happened in other functions vcpkg_configure_*.

@Neumann-A
Copy link
Contributor

VCPKG_CMAKE_CONFIGURE_OPTIONS is meant to splash onto vcpkg_list since vcpkg_list handles all elements correctly.
If it would have been just list() the extra quotes would have been necessary.

@PhoebeHui PhoebeHui changed the title Fix the application of VCPKG_CMAKE_CONFIGURE_OPTIONS with more than one option [vcpkg-cmake] Fix the application of VCPKG_CMAKE_CONFIGURE_OPTIONS with more than one option Jun 8, 2022
@TheJCAB
Copy link
Member Author

TheJCAB commented Jun 8, 2022

@Neumann-A "If it would have been just list() the extra quotes would have been necessary" - Necessary in order to allow options with (quoted) semicolons in them? I'm still wrapping my head around this.

@dg0yt "insight in some of the quirks resolved by the function" - The unit test shows the behaviors that the function is supposed to implement, but not the differences with CMake's own list() function (which is not really documented to this amount of detail).

Thanx all!

@TheJCAB
Copy link
Member Author

TheJCAB commented Jun 9, 2022

I guess I don't have write access to merge this? Appreciate it if someone else does it before conflicts creep in.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jun 10, 2022
@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Jun 10, 2022
In my defence, I'm new to this...
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 14, 2022
@dan-shaw dan-shaw merged commit 61e686b into microsoft:master Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants