Skip to content

Improved package : petsc#517

Merged
tgamblin merged 2 commits intospack:developfrom
epfl-scitas:packages/petsc
Mar 9, 2016
Merged

Improved package : petsc#517
tgamblin merged 2 commits intospack:developfrom
epfl-scitas:packages/petsc

Conversation

@alalazo
Copy link
Copy Markdown
Member

@alalazo alalazo commented Mar 9, 2016

Modifications :
  • added variants for mpi, shared, double (floating point precision)
  • added variants for a bunch of supported dependencies
Note :

After do_install the build will possibly fail with an error that is unrelated to the package modifications:

Traceback (most recent call last):
  File "/home/culpo/github/spack/bin/spack", line 176, in <module>
    main()
  File "/home/culpo/github/spack/bin/spack", line 154, in main
    return_val = command(parser, args)
  File "/home/culpo/github/spack/lib/spack/spack/cmd/install.py", line 81, in install
    fake=args.fake)
  File "/home/culpo/github/spack/lib/spack/spack/package.py", line 946, in do_install
    spack.hooks.post_install(self)
  File "/home/culpo/github/spack/lib/spack/spack/hooks/__init__.py", line 69, in __call__
    hook(pkg)
  File "/home/culpo/github/spack/lib/spack/spack/hooks/sbang.py", line 75, in post_install
    if shebang_too_long(path):
  File "/home/culpo/github/spack/lib/spack/spack/hooks/sbang.py", line 39, in shebang_too_long
    with open(path, 'r') as script:
IOError: [Errno 21] Is a directory: '/home/culpo/github/spack/opt/spack/x86_E5v2_IntelIB/gcc-4.4.7/petsc-3.6.3-oaidkcmi2x6fggx2dclhu6lh63elebvi/bin/julia'

I didn't solve that yet and I am not sure if we want it to be part of this PR. I assume it may be related to #497.

tgamblin
tgamblin reviewed Mar 9, 2016
View reviewed changes
var/spack/repos/builtin/packages/petsc/package.py
depends_on('boost', when='+boost')
depends_on('metis', when='+metis')

depends_on('hdf5~cxx~unsupported+mpi', when='+hdf5+mpi')
Copy link
Copy Markdown
Member

@tgamblin tgamblin Mar 9, 2016

Choose a reason for hiding this comment

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

does petsc conflict with hdf5+cxx or does it just not need the C++ bindings? Likewise for unsupported -- would depends_on('hdf5+mpi', when='+hdf5+mpi') be enough or does that cause issues?

Copy link
Copy Markdown
Contributor

@eschnett eschnett Mar 9, 2016

Choose a reason for hiding this comment

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

I'm building PETSc with hdf5+cxx+unsupported, and it's working fine. I think this needs changing.

Copy link
Copy Markdown
Member

@tgamblin tgamblin Mar 9, 2016

Choose a reason for hiding this comment

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

I merged it -- want to just submit another PR?

Copy link
Copy Markdown
Member Author

@alalazo alalazo Mar 9, 2016

Choose a reason for hiding this comment

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

@eschnett For me hdf5+cxx+unsupported works with anything but Intel. That's why I used the supported version.

Copy link
Copy Markdown
Member

@tgamblin tgamblin Mar 9, 2016

Choose a reason for hiding this comment

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

Would it make sense to put depends_on('hdf5+mpi', when='+mpi') here and make the unsupported variant in hdf5 default to False? If it breaks with intel we probably don't want it to be enabled by default.

Copy link
Copy Markdown
Contributor

@eschnett eschnett Mar 9, 2016

Choose a reason for hiding this comment

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

unsupported in HDF5's configurations doesn't mean that it enables dodgy code. It only enables additional configuration options, or rather combinations of configuration options. Thus, if something doesn't work with Intel, something else needs to be disabled, and unsupported can remain enabled.

Here, it seems that the Intel compiler can't handle HDF5's C++ interface.

Which version of the Intel compiler is this? What is the error message? What system is this?

If we disable something, we should keep a record of why this was done.

Copy link
Copy Markdown
Member

@tgamblin tgamblin Mar 9, 2016

Choose a reason for hiding this comment

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

@alalazo: you actually don't (necessarily). depends_on('hdf5+mpi') is not the same as depends_on('hdf5+mpi~cxx'). The first can link against an hdf5 that satisfies depends_on('hdf5+mpi+cxx'), if it's available. The second cannot.

Copy link
Copy Markdown
Contributor

@eschnett eschnett Mar 9, 2016

Choose a reason for hiding this comment

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

The current PETSc dependency means that no one can use PETSc and C++-HDF5 at the same time.

Copy link
Copy Markdown
Member Author

@alalazo alalazo Mar 9, 2016

Choose a reason for hiding this comment

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

@eschnett I think my position on having unsupported as a default is quite clear so I won't argue on that again 😄

@tgamblin It was just a choice of convenience : petsc doesn't need +cxx and having the spec restricted permits to always use the default without appending something like:

spack install petsc%intel ^ hdf5~cxx~unsupported

If you think it is better having it less restricted it's fine with me.

Copy link
Copy Markdown
Member

@tgamblin tgamblin Mar 9, 2016

Choose a reason for hiding this comment

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

I think less restricted is good. I think we can work on the inconvenience aspect when PR #120 is merged, which I need to get back to 😄. Now to resolve this unsupported thing...

tgamblin added a commit that referenced this pull request Mar 9, 2016
@tgamblin
Merge pull request #517 from epfl-scitas/packages/petsc
ca34388 
Improved package : petsc
@tgamblin tgamblin merged commit ca34388 into spack:develop Mar 9, 2016
@alalazo alalazo deleted the packages/petsc branch March 9, 2016 18:37
tgamblin added a commit that referenced this pull request Mar 9, 2016
@tgamblin
Merge @citibeth and @alalazo's petsc fixes from #515 and #517
b43c277
tgamblin referenced this pull request Mar 9, 2016
@tgamblin
Make unsupported variant default to False.
70aa650
@citibeth citibeth mentioned this pull request Mar 9, 2016
Merged
@tgamblin tgamblin mentioned this pull request Mar 9, 2016
Merged
tgamblin added a commit that referenced this pull request Mar 9, 2016
@tgamblin
Merge pull request #519 from LLNL/features/more-petsc-fixes
e6a3468 
Merge @citibeth and @alalazo's petsc fixes from #515 and #517
@alalazo alalazo mentioned this pull request Mar 9, 2016
Closed
matz-e added a commit to matz-e/spack that referenced this pull request Apr 27, 2020
@matz-e
synapsetool: add 0.5.3 (spack#517)
39b3f5a
climbfuji added a commit to climbfuji/spack that referenced this pull request Feb 19, 2025
@climbfuji
Merge pull request spack#517 from climbfuji/feature/py-maturin_py-rpd…
5595f81 
…s-py_rust-bootstrap

Add rust-bootstrap variant to py-maturin and py-rpds-py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alalazo @eschnett @tgamblin