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

Conversation

smacfarlane
Copy link
Contributor

This PR resolves downstream build errors in Drupal

https://bldr.habitat.sh/#/pkgs/core/drupal/jobs/1103619923366256640

Signed-off-by: Scott Macfarlane smacfarlane@chef.io

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
@smacfarlane smacfarlane requested a review from a team as a code owner October 29, 2018 16:28
@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@smacfarlane smacfarlane changed the title Add gcc-libs to pkg_deps [php] Add gcc-libs to pkg_deps Oct 29, 2018
Copy link
Collaborator

@predominant predominant left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@universam1 universam1 left a comment

Choose a reason for hiding this comment

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

consider the comment above why the LD_LIBRARY_PATH is actually necessary

# 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.

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
@smacfarlane smacfarlane merged commit 4db2ffc into master Nov 26, 2018
@smacfarlane smacfarlane deleted the sm/fix-php-downstream-builds branch November 26, 2018 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants