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

fix vcpkg_configure_cmake for case when having semicolons in OPTIONS #12977

Merged
merged 13 commits into from
Aug 24, 2020

Conversation

jgehw
Copy link
Contributor

@jgehw jgehw commented Aug 18, 2020

fixes #4320
after having fixed #12926 this was an easy one (special case of root cause in #12926)

jgehw and others added 2 commits August 17, 2020 15:58
incorporate changes from microsoft:master
@Neumann-A
Copy link
Contributor

Does it depend on #12926?

@jgehw
Copy link
Contributor Author

jgehw commented Aug 18, 2020

Does it depend on #12926?

No, it's independent. It's only the same type of bug, resulting from the undocumented difference between the two cmake API variants
cmake_parse_arguments(<prefix> <options> <one_value_keywords> <multi_value_keywords> <args>...)
cmake_parse_arguments(PARSE_ARGV <N> <prefix> <options> <one_value_keywords> <multi_value_keywords>)
w.r.t. to handling semicolons. The fix was rather easy in this case because only non-semicolon-destructive list(APPEND ...) operations (no list(INSERT ...) etc.) are applied after arguments parsing.

@Neumann-A
Copy link
Contributor

resulting from the undocumented difference between the two cmake API

It is not undocumented:

The PARSE_ARGV signature is only for use in a function() body. In this case the arguments that are parsed come from the ARGV# variables of the calling function. The parsing starts with the -th argument, where is an unsigned integer. This allows for the values to have special characters like ; in them.

otherwise cmake will treat ; as a list separator since ARGN is parsed as a list.

@jgehw
Copy link
Contributor Author

jgehw commented Aug 19, 2020

It is not undocumented

You're right, my fault, I didn't read carefully enough.

@jgehw
Copy link
Contributor Author

jgehw commented Aug 20, 2020

merged master because PR #13000 contains CI baseline fixes for soil, which just broke on invalid repository URL while this PR was work in progress

@jgehw
Copy link
Contributor Author

jgehw commented Aug 20, 2020

@Neumann-A can you as the author of lapack-reference see why this is failing, e.g., on x64_uwp? I looked through the sources of your portfiles, found one usage of vcpkg_configure_cmake, but as far as I understand it, none of its options has a semicolon it, so I can't see how this PR could break things with lapack-reference. Also, in the log output I didn't find a reason. I'm just wondering if the log output ends abruptly? The last lines are:
-- Configuring x64-uwp
-- Building x64-uwp-dbg
-- Building x64-uwp-rel
-- Fixing pkgconfig file: D:/packages/lapack-reference_x64-uwp/lib/pkgconfig/blas.pc
-- Fixing pkgconfig file: D:/packages/lapack-reference_x64-uwp/lib/pkgconfig/lapack.pc
-- Fixing pkgconfig file: D:/packages/lapack-reference_x64-uwp/debug/lib/pkgconfig/blas.pc
-- Fixing pkgconfig file: D:/packages/lapack-reference_x64-uwp/debug/lib/pkgconfig/lapack.pc
-- Warning: Could not find a matching pdb file for:
D:/packages/lapack-reference_x64-uwp/bin/liblapack.dll
D:/packages/lapack-reference_x64-uwp/debug/bin/liblapack.dll

-- Installing: D:/packages/lapack-reference_x64-uwp/share/lapack-reference/copyright

@Neumann-A
Copy link
Contributor

- Performing post-build validation
Import libs were not present in D:\packages\lapack-reference_x64-uwp\debug\lib
If this is intended, add the following line in the portfile:
    SET(VCPKG_POLICY_DLLS_WITHOUT_LIBS enabled)
Import libs were not present in D:\packages\lapack-reference_x64-uwp\lib
If this is intended, add the following line in the portfile:
    SET(VCPKG_POLICY_DLLS_WITHOUT_LIBS enabled)
Found 2 error(s). Please correct the portfile:
    C:\agent\_work\1\s\ports\lapack-reference\portfile.cmake
-- Performing post-build validation done

You are probably not passing FORTRAN_CMAKE correctly forward

@BillyONeal
Copy link
Member

@jgehw It seems like "${FORTRAN_CMAKE}" should not have quotes around it in ports\lapack-reference\portfile.cmake; you're fixing a bug that some other ports are depending on. (Load bearing bugs are fun eh?)

jgehw added 4 commits August 21, 2020 21:36
- ${FORTRAN_CMAKE} is a list, which doesn't get expanded when quoted
- previously this didn't hurt because due to the bug in vcpkg_configure_make() everything, even strings containing semicolons, got expanded as list
@jgehw
Copy link
Contributor Author

jgehw commented Aug 21, 2020

@jgehw It seems like "${FORTRAN_CMAKE}" should not have quotes around it in ports\lapack-reference\portfile.cmake; you're fixing a bug that some other ports are depending on. (Load bearing bugs are fun eh?)

@BillyONeal Indeed, reading the sources, I found out that ${FORTRAN_CMAKE} is a list, not a literal; this list needs to get expanded, which doesn't happen, when quoted. Prior to the fix of this PR, this didn't matter because the bug fixed in this PR treated everything as list, even strings with semicolons in it.

@BillyONeal BillyONeal merged commit 02bfa29 into microsoft:master Aug 24, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

@jgehw jgehw deleted the fix-vcpkg-configure-cmake branch August 24, 2020 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants