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

adding perl dependency to json-c #11631

Closed
wants to merge 2 commits into from
Closed

adding perl dependency to json-c #11631

wants to merge 2 commits into from

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Jun 5, 2019

This will address a small error that the default perl is too old to support json-c. If the user hasn't installed anything with perl recently, it will trigger an error. The error was reported in #11613 which is closed, but the issue remains with the json-c package. The user fixed by installing 2.28, but I chose the earliest version of perl with a hash provided here. @ziya reported other issues with installing packages that depend on a newer version of perl, and (although he didn't share) these should also be looked into.

Signed-off-by: Vanessa Sochat vsochat@stanford.edu

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Copy link
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

Is this issue really with json-c, or is this an issue with something in autoconf conflicting with the default Perl version? I'd like to understand better what's causing the problem, as it looks like this is some issue with autoconf's perl dependency, not json-c's.

I quickly googled the error and found this issue:
https://bugzilla.redhat.com/show_bug.cgi?id=924882

It looks like a similar error came up in Fedora because Perl didn't have Carp. Does autoconf just need a dependency on some version of perl that support that (or on perl-carp-clan)? Maybe @hartzell knows.

@@ -16,6 +16,7 @@ class JsonC(AutotoolsPackage):
version('0.11', 'aa02367d2f7a830bf1e3376f77881e98')

depends_on('autoconf', type='build')
depends_on('perl@5.16.3')
Copy link
Member

Choose a reason for hiding this comment

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

This is too specific -- it'll force every thing that builds with json-c to use Perl 5.16.3. If this is the minimum version needed it should say 5.16.3:

I'm also nearly positive this is a build dependency inherited through autoconf, so it needs to be specified (by default it's build/link and will add RPATHs for perl libs)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to be a minimum version of 5.16.3:. I can't comment on if it's autoconf related, so I'll leave it here for now for @hartzell to comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Autoconf/etc... require perl when they run. I haven't found a firm statement of what version they require, the best is this autotools announcement from 2012 that mentions that autotools now require perl 5.6 or better.

Ah, here's a bit from autoconf's own configure.ac, so 5.6 still looks right (keeping in mind perl's internal [5.006] vs external [5.6] version representation...).

140 $PERL -e 'require 5.006;' || {
141    AC_MSG_ERROR([Perl 5.006 or better is required])
142 }

I can't replicate the failure on my CentOS boxes, but I think that's because they have a decent enough /usr/bin/perl. The module that @ziya5635 had trouble with is strict.pm which is /usr/share/perl5/strict.pm for the CentOS system perl. It's a fundamental bit of the perl core, I can't imagine it going missing.... I've asked @ziya5635 for additional info about how he was triggering his issue.

I'd be inclined to do depends_on('perl', type='run') and get fancier when there's data. If we ever add a perl older than 5.6 to the tree, we have other problems... 😱

I'll follow up if I can recreate the bug in #11613.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @hartzell you're awesome! I too wasn't able to reproduce the issue, I'm not on Ubuntu 18.04 (I upgraded!)

Copy link
Contributor

Choose a reason for hiding this comment

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

An edit, to my earlier comment, I'd be inclined to add a depends_on('perl', type='run') to autoconf.

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented Jun 6, 2019

@hartzell do you mean in the package.py for autoconf? It looks like a build and run dependency for perl is there:

    # Note: m4 is not a pure build-time dependency of autoconf. m4 is
    # needed when autoconf runs, not only when autoconf is built.
    depends_on('m4@1.4.6:', type=('build', 'run'))
    depends_on('perl', type=('build', 'run'))

or somewhere else?

@hartzell
Copy link
Contributor

hartzell commented Jun 6, 2019

[edit, fixed @ziya5635's handle]

Yep, that's what I mean. Errr, well, meant. That's what I get for not checking....

That should be sufficient. json-c needs autoconf to build, and autoconf needs perl to run.

If I dig out the perl search path (@INC) from the original report in #11613 and clean it up a bit, I get this, which is consistent with the existing dependencies for json-c and autoconf:

/home/spack/spack/opt/spack/linux-ubuntu16.04-x86_64/gcc-7.2.0/autoconf-2.69-yb2makbkwt434wy6zoxk64rx5mpea3aj/share/autoconf
/home/ubuntu/spack/opt/spack/linux-ubuntu16.04-x86_64/gcc-7.2.0/perl-5.26.2-fdwz5yu6mbtyvdu7etbed25s24s535es/lib/perl5
/home/ubuntu/spack/opt/spack/linux-ubuntu16.04-x86_64/gcc-7.2.0/perl-5.26.2-fdwz5yu6mbtyvdu7etbed25s24s535es/lib/site_perl/5.26.2/x86_64-linux-thread-multi
/home/ubuntu/spack/opt/spack/linux-ubuntu16.04-x86_64/gcc-7.2.0/perl-5.26.2-fdwz5yu6mbtyvdu7etbed25s24s535es/lib/site_perl/5.26.2
/home/ubuntu/spack/opt/spack/linux-ubuntu16.04-x86_64/gcc-7.2.0/perl-5.26.2-fdwz5yu6mbtyvdu7etbed25s24s535es/lib/5.26.2/x86_64-linux-thread-multi
/home/ubuntu/spack/opt/spack/linux-ubuntu16.04-x86_64/gcc-7.2.0/perl-5.26.2-fdwz5yu6mbtyvdu7etbed25s24s535es/lib/5.26.2

When I install perl@5.26.2, strict.pm is installed in the expected location:

$ find /home/ghartzell/tmp/spack/opt/spack/linux-centos7-x86_64/gcc-4.8.5/perl-5.26.2-7tmxubxk4qnty7decxqsz5yu5v3p3lqq | grep strict
/home/ghartzell/tmp/spack/opt/spack/linux-centos7-x86_64/gcc-4.8.5/perl-5.26.2-7tmxubxk4qnty7decxqsz5yu5v3p3lqq/lib/5.26.2/strict.pm
/home/ghartzell/tmp/spack/opt/spack/linux-centos7-x86_64/gcc-4.8.5/perl-5.26.2-7tmxubxk4qnty7decxqsz5yu5v3p3lqq/man/man3/strict.3

I'll wait until @ziya5635 lets me know what he actually ran and I'll see if I can recreate whatever problem he had. At the moment I'm officially confused.

@alalazo
Copy link
Member

alalazo commented Aug 19, 2020

Closing as stale. Feel free to reopen if you disagree. Sidenote: in the meanwhile json-c passed to CMake as a build system

@alalazo alalazo closed this Aug 19, 2020
@vsoch
Copy link
Member Author

vsoch commented Aug 19, 2020

Some feedback on the review - it would have been nice to have been involved in the re-implementation instead of telling me to wait on something and closing with a”stale” comment a year 2 months later... :/

@alalazo
Copy link
Member

alalazo commented Aug 19, 2020

Apologies @vsoch I'm just going through old PRs that appear to be stale and trying to clean up our backlog. Reopened in case you want to continue working on it.

@alalazo alalazo reopened this Aug 19, 2020
@alalazo
Copy link
Member

alalazo commented Aug 19, 2020

it would have been nice to have been involved in the re-implementation instead of telling me to wait on something and closing with a”stale” comment a year 2 months later... :/

I might not have been as clear as I could. json-c passed to CMake for newer versions but our package still models old versions based on autotools. The re-implementation is up for grabs if you feel like doing it.

@vsoch
Copy link
Member Author

vsoch commented Aug 19, 2020

Yes! Quick question - are there any examples with spack with multiple install bases, or is it customary to create a different name / folder for the newer package? The older version is Autotools, the newer one, as you mentioned, is Cmake.

@alalazo
Copy link
Member

alalazo commented Aug 19, 2020

@vsoch Unfortunately we didn't come to a final agreement so far on what is THE strategy to employ in cases like this. There are discussions spread over a few issues/prs, like #15420 and #15896. Currently most packages do one of these two things:

  1. Inherit from Package and re-implement the logic needed for the build-systems to be supported (involves a bit of code duplication, usually works for simple packages)
  2. Suffix the name of the package with the old build-system with a -legacy and re-implement the package with the new build system

A case for 1 is e.g. atk that passed from a custom (I guess) configure to meson while cases for 2 are:

$ spack list legacy
==> 4 packages.
blast-legacy  kokkos-kernels-legacy  kokkos-legacy  singularity-legacy

Oh, there have been also a few cases of low-traffic packages where the maintainer decided it was not worth keeping the old versions around and just changed everything to the new build system.

@vsoch
Copy link
Member Author

vsoch commented Aug 19, 2020

okay thanks! I can definitely try 1 first (looking for examples for guidance). This PR is quite old so it will be easier to start with a new branch.

@vsoch vsoch closed this Aug 19, 2020
@vsoch vsoch deleted the update/json-c-deps branch August 19, 2020 16:15
@vsoch
Copy link
Member Author

vsoch commented Aug 24, 2020

hey @alalazo I'm starting to look at packages and the difference between AutoTools and Cmake. Could you show me an example that you mentioned for point 1 - having custom logic for a base Package? For example, I'd want to be able to do something like:

if less than this version, we want Autotools, do:
    phases = ['autoreconf', 'configure', 'build', 'install']
otherwise, cmake
    phases = ['cmake', 'build', 'install']

@alalazo
Copy link
Member

alalazo commented Aug 25, 2020

Sure @vsoch. You can have a look at arpack-ng:

$ spack edit arpack-ng

for an example of 1. Unfortunately it doesn't work exactly like you would (which would be the optimal case) but relies on inheriting from the generic Package class that has only an install phase and reimplement the installation procedure within that phase. This is due to some core design decisions that need to be revisited to "delay" selecting a build system until after concretization is finished.

@vsoch
Copy link
Member Author

vsoch commented Aug 25, 2020

I don't think I understand enough how this works to work on this, sorry @alalazo. But thanks for your help!

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