Skip to content

ext/zip: Remove un-needed check for zip_open #4189

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

Closed
wants to merge 2 commits into from

Conversation

hughmcmaster
Copy link
Contributor

@hughmcmaster hughmcmaster commented May 26, 2019

Updated per review feedback and upstream changes.

@hughmcmaster hughmcmaster changed the title Raise required version of libzip and replace calls to PHP_ADD_LIBRARY_WITH_PATH Replace calls to PHP_ADD_LIBRARY_WITH_PATH and remove extra libs variables May 29, 2019
@hughmcmaster
Copy link
Contributor Author

@nikic, this is waiting for a review whenever you get time.

@nikic
Copy link
Member

nikic commented May 30, 2019

@hughmcmaster As said before, I'm not sure that removing those -L$LIBZIP_LIBDIR is correct and expect that it will fail for shared builds and we need to use $ZIP_SHARED_LIBADD there instead. Can you check whether --with-zip=shared works correctly after these changes?

@hughmcmaster hughmcmaster changed the title Replace calls to PHP_ADD_LIBRARY_WITH_PATH and remove extra libs variables ext/zip: Remove un-needed check for zip_open Jun 2, 2019
@hughmcmaster
Copy link
Contributor Author

hughmcmaster commented Jun 2, 2019

This version removes the un-necessary zip_open symbol check and adds PHP_EVAL_LIBLINE to match all other pkg-config-based extensions. This means PHP_ADD_LIBRARY_WITH_PATH is no longer needed.

A future PR will replace $LIBZIP_LIBDIR with $LIBZIP_LIBS.

Thank you to everyone for your patience and explanations on the issues in the previous PR versions.

@php-pulls php-pulls closed this in f349d79 Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants