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

[php] Add gcc-libs to pkg_deps #1946

Merged
merged 3 commits into from Nov 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 2 additions & 5 deletions php/plan.sh
Expand Up @@ -23,6 +23,7 @@ pkg_deps=(
core/readline
core/zip
core/zlib
core/gcc-libs
)
pkg_build_deps=(
core/autoconf
Expand All @@ -38,11 +39,7 @@ pkg_include_dirs=(include)
pkg_interpreters=(bin/php)

do_build() {
# The configuration scripts unset LD_RUN_PATH when testing linking for configured options,
# so the resulting 'conftest' binaries cannot run due to being unable to find libstdc++.so
# This allows those binaries to execute while limiting the scope of LD_LIBRARY_PATH
# to the execution of `./configure`.
LD_LIBRARY_PATH="$(pkg_path_for gcc)/lib" ./configure --prefix="${pkg_prefix}" \
./configure --prefix="${pkg_prefix}" \

Choose a reason for hiding this comment

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

this will break libzip support!

Suggested change
./configure --prefix="${pkg_prefix}" \
LD_LIBRARY_PATH="$(pkg_path_for gcc)/lib" ./configure --prefix="${pkg_prefix}" \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote the comment, and it's wrong 😞. The underlying issue was that core/gcc-libs wasn't in the pkg_deps therefore the conftest binaries were unable to execute, not that the path was being manipulated.

The above LD_LIBRARY_PATH modifies only the runtime for ./configure and has no effect on commands executed later in the build, or in the runtime for the php package. Since this change adds core/gcc-libs to the pkg_deps, ./configure is able complete successfully without setting LD_LIBRARY_PATH, so LD_LIBRARY_PATH and the comment can safely be removed.

I've put together a small test case to validate. Note: I'm not a php dev, so suggestions on correctness are welcome 😄

Given the following php:

<?
  $za = new ZipArchive;
  $za->open('test.zip',ZipArchive::CREATE|ZipArchive::OVERWRITE);
  $za->addFile("php/plan.sh");
  $za->close();
?>

When executing it under core/php/7.2.8/20181008145855 (current stable):

[31][default:/src:0]# hab pkg exec core/php/7.2.8/20181008145855 php test.php

Fatal error: Uncaught Error: Class 'ZipArchive' not found in /src/test.php:2
Stack trace:
#0 {main}
  thrown in /src/test.php on line 2

When executed it on a build with these changes:

[32][default:/src:0]# hab pkg exec smacfarlane/php php test.php
[33][default:/src:0]# unzip -l test.zip
Archive:  test.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
     2027  2018-10-30 14:45   php/plan.sh
---------                     -------
     2027                     1 file

Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉 👍 💯 🥇

Copy link
Collaborator

Choose a reason for hiding this comment

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

@smacfarlane How about adding these steps to a test we can include? That would prevent any zip regression in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@predominant Done!

@universam1 Does the above address your concerns?

Choose a reason for hiding this comment

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

Cool, I’ll try!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@universam1 I'm going to merge this, please open an issue if the resulting packages do not contain libzip support.

--enable-exif \
--enable-fpm \
--with-fpm-user=hab \
Expand Down
5 changes: 5 additions & 0 deletions php/test/libzip_support.php
@@ -0,0 +1,5 @@
<?
# The below will exit with a fatal error if libzip is not supported.
$za = new ZipArchive;
echo("Libzip is supported.\n");
?>
20 changes: 20 additions & 0 deletions php/test/test.sh
@@ -0,0 +1,20 @@
#!/bin/bash

set -euo pipefail

if [[ -z "${1:-}" ]]; then
echo "Usage: test.sh <fully qualified package ident>"
echo
echo "ex: test.sh core/php/7.2.8/20181108151533"
exit 1
fi

pkg_ident=${1}

# This depends on the package being present on the depot OR
# in your local cache. If you're testing locally this is a
# reasonably safe assumption to make, as long as the cache
# isn't cleaned aggressively
hab pkg install "$pkg_ident"

hab pkg exec "$pkg_ident" php "$(dirname "$0")"/libzip_support.php