-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[openssl,vcpkg-cmake-get-vars] Fix android and emscripten, add feature tools
#29966
Conversation
System variables like 'APPLE' or 'ANDROID' aren't set before 'project()'.
@dg0yt |
@dg0yt, thank you. I'll try to test this during next two days. P.S. Sorry I haven't seen you was working on that PR since October. Unfortunately, I don't track all changes that are happening on vcpkg. I only see smth that breaks my CI. |
af8102b
to
3c17992
Compare
This fix doesn't work for android:
one of the errors in the log:
|
Show |
armv6 not added as a community triplet and support added here: |
Again, show
|
Got it from your zip:
Obviously broken. But I'm not convinced that this is related to this PR. |
it working with ndk r15c |
This is a suggestion for fix? |
I added this workaround for armv6-android triplet, and I get the same error. |
armv6 was working before your change here: |
No. I was only pointing at the line which was reported as syntax error.
This is not how things work. And if we know that the syntax is broken, it is probably not helpful to add it anywhere. The error can be reproduced in master with |
So it is a regression from this PR. But the issue is not in the scripts modified by that PR but used since that PR. |
Maybe helpful note: vcpkg might to pass compiler flags via CC etc instead of CFLAGS etc. in the future. So you can just deactivate whatever the internal buildsystem of a port would add. |
For openssl, it doesn't help too much because it ignores CC for some triplets. |
tools
tools
@dg0yt Thank you for these fixes. I can confirm that it working for all android triplets and wasm32-emscripten. |
@m-kuhn Do you openssl on android, ios or emscripten? (In particular ios wasn't tested yet AFAICT.) |
Ios and android (r25), yes. Should I give this PR a testrun? |
@m-kuhn That would be great.. There is some room for unintented side effects. |
It builds and links fine, thank you for asking. |
May I ask why we put this pr on hold? |
@Neumann-A, any chance to get this merged this week? |
@AenBleidd i don't have any power here so i am the wrong person to mention for that. |
Fixes #29947. (Android regression from #27261.)
Fixes #30037. Fixes #29575. (wasm32-emscripten, broken before #27261.)
Fixes #29674. (arm64-linux, broken before #27261.)
The
openssl
tool is now an opt-in feature. Resolves #29948. (Not a regression, but changed by #27261.)Community test success reported for android, ios, wasm-emscripten and mingw triplets.
While fixing Android, I noticed that
vcpkg-cmake-get-vars
was probably not working correctly with regard to osx deployment targets - we cannot rely onAPPLE
orANDROID
being set before callingproject
.Testing also revealed insufficent handling of quotes in
vcpkg-cmake-get-vars
with regard to the generated cmake files. This PR adds uniform escaping, and it ports relevant changes fromvcpkg-cmake-get-vars
to the global script.Finally it turned out that additional quoting fixes were needed with regard to passing options to tools in
vcpkg_configure_make
.Last not least, it generally fixes libpq's openssl version detection in a way which doesn't need to run the (target:zap:) openssl tool.
./vcpkg x-add-version --all
and committing the result.