Skip to content

Update for 'netcdf'.#5819

Merged
tgamblin merged 1 commit intospack:developfrom
skosukhin:netcdf
Oct 25, 2017
Merged

Update for 'netcdf'.#5819
tgamblin merged 1 commit intospack:developfrom
skosukhin:netcdf

Conversation

@skosukhin
Copy link
Copy Markdown
Member

@skosukhin skosukhin commented Oct 19, 2017

This is an update for NetCDF.

There are several questions that need to be answered before this gets merged:

  1. There was a workaround introduced by @adamjstewart in Fix NetCDF/HDF5 dependency resolution problems #1553. Do we still need it? I tried to run the commands from the discussion and they seem to work.
  2. What do we do with options 'parallel4' and 'pnetcdf'? Should it be just one option 'mpi' that enables both parallel I/O features? If not, what should be the default values for these variants? Both set to True? See also netcdf: multiple improvements #2377.
  3. Should the package depend on 'mpi'? From one hand, we should run the MPI wrapper. From another hand, netcdf itself does not need mpi and self.spec['mpi'].mpicc is set for hdf5/pnetcdf anyway and we can use it.

davydden
davydden approved these changes Oct 19, 2017
View reviewed changes
var/spack/repos/builtin/packages/netcdf/package.py Outdated

# High-level API of HDF5 1.8.9 or later is required for netCDF-4 support:
# http://www.unidata.ucar.edu/software/netcdf/docs/getting_and_building_netcdf.html
# TODO: change to 'hdf5+hl' when/if https://github.com/LLNL/spack/pull/5790
Copy link
Copy Markdown
Member

@davydden davydden Oct 19, 2017

Choose a reason for hiding this comment

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

when/if is merged ?

Copy link
Copy Markdown
Member Author

@skosukhin skosukhin Oct 19, 2017

Choose a reason for hiding this comment

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

There is no 'hl' variant for hdf5 in the develop branch. The referenced PR introduces it.

Copy link
Copy Markdown
Member

@davydden davydden Oct 19, 2017

Choose a reason for hiding this comment

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

i know, i am saying you miss is merged in the sentence.

Copy link
Copy Markdown
Member Author

@skosukhin skosukhin Oct 19, 2017

Choose a reason for hiding this comment

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

Ok, done.

var/spack/repos/builtin/packages/netcdf/package.py Outdated
LDFLAGS.append("-L%s/lib" % spec['parallel-netcdf'].prefix)
# undefined reference to `SSL_CTX_use_certificate_chain_file
curl = self.spec['curl']
curl_libs = self.spec['curl'].libs
Copy link
Copy Markdown
Member

@davydden davydden Oct 19, 2017

Choose a reason for hiding this comment

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

curl_libs = curl.libs (since you introduce curl above)

Copy link
Copy Markdown
Member Author

@skosukhin skosukhin Oct 19, 2017

Choose a reason for hiding this comment

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

Fixed.

@skosukhin
Copy link
Copy Markdown
Member Author

skosukhin commented Oct 19, 2017

I can't compile with +cdmremote (even if I try it on the develop branch of Spack). It works only if --disable-netcdf4 but it's not what we want. Could somebody try it? Should we keep this variant since it was removed starting version 4.4 anyway?

@skosukhin
Copy link
Copy Markdown
Member Author

skosukhin commented Oct 19, 2017

I have managed to reproduce the problem described in #1553. Looks like packages.yaml gets accounted for in a wrong time. Added comments and replaced the workaround with conflicts() statements.

@aprokop aprokop removed their request for review October 19, 2017 15:27
@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Oct 19, 2017

There was a workaround introduced by @adamjstewart in #1553. Do we still need it? I tried to run the commands from the discussion and they seem to work.

There have been no changes to the concretizer as far as I know, so I'm going to say yes.

@skosukhin skosukhin changed the title [WIP] Update for 'netcdf'. Update for 'netcdf'. Oct 25, 2017
@skosukhin
Copy link
Copy Markdown
Member Author

skosukhin commented Oct 25, 2017

I would say that this is ready for merging.

@tgamblin tgamblin merged commit c9810f8 into spack:develop Oct 25, 2017
@davydden davydden mentioned this pull request Oct 30, 2017
Merged
@skosukhin skosukhin deleted the netcdf branch November 7, 2017 14:13
@skosukhin skosukhin mentioned this pull request May 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalazo alalazo Awaiting requested review from alalazo

@adamjstewart adamjstewart Awaiting requested review from adamjstewart

1 more reviewer

@davydden davydden davydden approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@skosukhin @adamjstewart @davydden @tgamblin