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

libcf #2334

Merged
merged 20 commits into from
Feb 18, 2017
Merged

libcf #2334

merged 20 commits into from
Feb 18, 2017

Conversation

dnadeau4
Copy link
Contributor

@dnadeau4 dnadeau4 commented Feb 2, 2017

Build libcf dependency for UV-CDAT.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/libcf) and found it was in an excellent condition.

@@ -0,0 +1,15 @@
export CFLAGS="-Wall -g -m64 -pipe -O2 -fPIC"
Copy link
Member

Choose a reason for hiding this comment

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

Please change these to: export CFLAGS="-Wall -g -m64 -pipe -O2 -fPIC $CFLAGS"
So the original flags are preserved.

Copy link
Contributor

Choose a reason for hiding this comment

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

hum... In general I'm reluctant to picking up the user's flags... Does your script sets CFALGS in any special ways?

Copy link
Member

Choose a reason for hiding this comment

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

Don't worry about that in conda-forge. In a normal build there should be nothing in there. However, we want the scripts to be generic enough so other people with different build systems can inject whatever they need there. (We even have packages that do that in conda-forge to ensure some OS X specific variables for clang are used.)

So if a user is modifying that they should know what they are doing.

export LFLAGS="-fPIC"

CONDA_LST=`conda list`
if [[ ${CONDA_LST}'y' == *'openmpi'* ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this should not be here. I believe @msarahan and @minrk are working on a way to track mpi variants with features and they can probably chime with the best practice for doing this.

For now I recommend you to choose one and avoid this if-clause.

Copy link
Contributor

@doutriaux1 doutriaux1 Feb 2, 2017

Choose a reason for hiding this comment

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

@msarahan @minrk I would love to hear back from you on this. It is indeed a thorny point with conda and our system. If you want to take adavantage of mpi then some software needs to be compiled in a special fashion. Easiest example is hdf5. If I want to take advantage on hdf5 mpi's capabilities then I need hdf5 to be compiled against mpi. And flavors probably matter to. The way we solve this in our system is that my meta.yaml are actually pre-processed by a prep_for_build.py script (see: https://github.com/UV-CDAT/conda-recipes/blob/master/prep_for_build.py ) that make sure the dependencies are then set properly. It does smell a lot like conda's feature thing but it's not really. The only way to support different implementation is to add the "feature" name to the package for example using the same meta.yaml.in I generate either a meta.yaml that will be producing "mypackage" or "mypackage-openmpi" I implemented a {} system similar to conda's [] to support this. It is not ideal.
In the case of conda-forge I'm ok with offering two different recipes one for the "Standard" build one for the "package-openmpi" or "package-mpch" etc... But having a conda-proper way of doing things would be much better.

Copy link
Member

Choose a reason for hiding this comment

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

The discussion of how to express which mpi implementation is used is mainly here. Features should work for this, it's mainly a question of deciding precisely how those features should be defined / tracked.

If there does need to be logic in build.sh, I'm not sure that such things are passed down to the build environment. Are they?

requirements:
build:
- hdf5
- libnetcdf 4.3.3.1
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this strict pinning for libnetcdf? Please see conda-forge pinning docs at:
https://github.com/conda-forge/staged-recipes/wiki/Pinned-dependencies

Copy link
Contributor

Choose a reason for hiding this comment

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

historical no need to. I'll fix this.

Copy link
Member

@ocefpaf ocefpaf left a comment

Choose a reason for hiding this comment

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

@doutriaux1 doutriaux1 mentioned this pull request Feb 2, 2017
@dnadeau4
Copy link
Contributor Author

dnadeau4 commented Feb 7, 2017

@ocefpaf Appveyor hung here as well!

@dnadeau4
Copy link
Contributor Author

@ocefpaf Yeah!!!


if [ "$(uname)" == "Darwin" ]; then
export CXXFLAGS="${CXXFLAGS} -fno-common"
export MACOSX_DEPLOYMENT_TARGET=$(sw_vers -productVersion | sed -E "s/([0-9]+\.[0-9]+).*/\1/")
Copy link
Member

Choose a reason for hiding this comment

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

This should be fixed in conda-forge to ensure compatibility with the rest of the packages we ship. Take a look at packages like the toolchain that defines this.

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 hope your activate/deactivate works. This is a hard one to compile on OSX.
toolchain

fi

CONDA_LST=`conda list`
if [[ ${CONDA_LST}'y' == *'openmpi'* ]]; then
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 not OK. We cannot rely on conda list. For conda-forge you need enforce openmpi and wait until the features for the other variants is resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ocefpaf @dnadeau4 let's take it out for now. We need to revisit the whole suite eventually to use conda features for this. As discussed in earlier conda-forge recipes.

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'll get rid of that!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like the best approach for now. (Please join the discussion for the features to help move this forward.)

requirements:
build:
- hdf5
- libnetcdf
Copy link
Member

Choose a reason for hiding this comment

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

Please check the pinning script and pin the version here.

See https://github.com/conda-forge/staged-recipes/wiki/Pinned-dependencies

Copy link
Contributor

Choose a reason for hiding this comment

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

@ocefpaf I'm not sure what you're asking for here. Do you want us to pin or just confirm it will work against this? Or run the pinning script? We currently build in house against cnda-forge versions of hdf5 and netcdf. Please let us know what's really wanted here. Thanks!

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 need to pin the version using the wiki page info

Copy link
Member

@ocefpaf ocefpaf Feb 18, 2017

Choose a reason for hiding this comment

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

pin according to the wiki I sent you. We need that to ensure ABI stability and package compatibility.

The pinnings are automatically updated when we update the dependencies.

@ocefpaf
Copy link
Member

ocefpaf commented Feb 17, 2017

@dnadeau4 there are still a few points we need to address before merging this to make this package OK for conda-forge.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/libcf) and found some lint.

Here's what I've got...

For recipes/libcf:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/libcf) and found it was in an excellent condition.


./configure --prefix=${PREFIX}
${PYTHON} setup.py install
if [ `uname` == Darwin ]; then install_name_tool -change /System/Library/Frameworks/Python.framework/Versions/2.7/Python @rpath/libpython2.7.dylib ${SP_DIR}/pycf/*.so ; fi
Copy link
Member

Choose a reason for hiding this comment

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

If this is a Python 2 only package we need a skip statement below.


build:
number: 1
skip: True # [win or py3k]
Copy link
Member

Choose a reason for hiding this comment

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

Ah. We already have it!

sha256: e40d3826ca2b47538b075f69f716798e255fe396fd3ae68e8b051be57a951bb4

build:
number: 1
Copy link
Member

Choose a reason for hiding this comment

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

Can you reset this to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

- clapack >=3.2.1
- curl
- ossuuid
- gcc
Copy link
Member

Choose a reason for hiding this comment

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

Do you need Fortran or libgmop for this package? If not you don't need the gcc package.

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 need Fortran in this package.

- clapack >=3.2.1
- curl
- ossuuid
- libgcc
Copy link
Member

Choose a reason for hiding this comment

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

If you are using gcc/libgcc only for Fortran you can try using the libgfortan package instead of libgcc.

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'll change it for libgfortran.

@ocefpaf ocefpaf merged commit ad5ed95 into conda-forge:master Feb 18, 2017
@dnadeau4
Copy link
Contributor Author

@ocefpaf for some reason, travis did not build the osx and return green. I don't have libcf for osx in libcf-feedstock. I created an issue, but thought we could re-trigger here. What should be done?

@ocefpaf
Copy link
Member

ocefpaf commented Feb 26, 2017 via email

@dnadeau4
Copy link
Contributor Author

dnadeau4 commented Mar 2, 2017

@ocefpaf If we can get travis.yml in feedstock to compile osx, that will make it possible for me to add cdms2. I have cdutil and genutil packages depending on cdms2 that is in my queue for conda-forge.

As I told @jakirkham I can also create a brand new PR in staged-recipe for this. conda-forge/conda-smithy#465

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants