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

Added recipe for kwant #1446

Merged
merged 34 commits into from
Nov 3, 2016
Merged

Added recipe for kwant #1446

merged 34 commits into from
Nov 3, 2016

Conversation

basnijholt
Copy link
Contributor

@basnijholt basnijholt commented Aug 31, 2016

@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/kwant) and found some lint.

Here's what I've got...

For recipes/kwant:

  • Failed to even lint the recipe (might be a conda-smithy bug) 😢

@basnijholt
Copy link
Contributor Author

@jakirkham could you give me advice on the following part:

requirements:
  build:
    - gcc
    - openblas  # [linux]
  run:
    - openblas  # [linux]
    - libgcc
    - libgfortran  # [linux]

Should I swap gcc with toolchain and omit libgcc? And what about openblas and gfortran`?

@basnijholt
Copy link
Contributor Author

basnijholt commented Aug 31, 2016

For recipes/kwant:

  • Failed to even lint the recipe (might be a conda-smithy bug) 😢

@pelson @msarahan any idea why this can have happened? This is the meta.yaml

Edit: This is fixed now, thanks to @ocefpaf.


package:
name: {{ name }}
version: "1.2.2" [py3k]
Copy link
Member

Choose a reason for hiding this comment

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

The linter is choking here. You can lint a recipe locally with conda smithy recipe-lint.

@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/kwant) and found it was in an excellent condition.

requirements:
build:
- python
- gcc
Copy link
Member

Choose a reason for hiding this comment

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

Please use toolchain instead.

@jakirkham
Copy link
Member

FWICT this is Python, C, and C++. However there is not Fortran that I can see. The following suggestions should be fine.

Also does this link to a BLAS directly? If so, mimic our NumPy recipe w.r.t. OpenBLAS.

@basnijholt
Copy link
Contributor Author

basnijholt commented Aug 31, 2016

There's no fortran code in kwant itself. However, we link against mumps, which is a fortran library. So usually you'd also have to link against libgfortran.

Also the package on my channel doesn't work without gfortran. Are there differences for packages compiled by myself and conda-forge?

@basnijholt
Copy link
Contributor Author

basnijholt commented Aug 31, 2016

Also does this link to a BLAS directly? If so, mimic our NumPy recipe w.r.t. OpenBLAS.

Yes, kwant directly links to BLAS, because it uses LAPACK. I'll take a look at the numpy recipe.

@basnijholt
Copy link
Contributor Author

basnijholt commented Aug 31, 2016

@jakirkham you mean that I should add:

  features:
    - blas_openblas  # [linux]

as I did here.

However, in the numpy recipe I also see - blas 1.1 openblas. Don't know what it's for, but should I include that as well?

Another thing, why is OpenBLAS also added for Mac OSX, I thought this wasn't needed?

@basnijholt basnijholt mentioned this pull request Oct 10, 2016
5 tasks

about:
home: http://kwant-project.org/
license: BSD 2-Clause
Copy link
Member

Choose a reason for hiding this comment

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

Could optionally add license_family: BSD.

Copy link
Member

Choose a reason for hiding this comment

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

Done below.

@basnijholt
Copy link
Contributor Author

basnijholt commented Nov 1, 2016

Are all of these needed at build time or can some of them be dropped (e.g. matplotlib, scipy, etc.)?

@jakirkham I have enumerated them all, and the current build requirements are all needed (I removed matplotlib.)

package:
name: {{ name }}
version: {{ py3k_ver }} # [py3k]
version: {{ py2k_ver }} # [py2k]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have 2 different versions? Could we not just start with 1.1.2 and then drop Python 2.x support when upgrading or is there more going on here?

Copy link
Contributor Author

@basnijholt basnijholt Nov 2, 2016

Choose a reason for hiding this comment

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

Development for Python 2 stopped, however, bug fixes will still be applied. So the version number for both Python 2 and 3 will change, but not won't be the same.

Copy link
Member

Choose a reason for hiding this comment

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

So the typical solution for this is to start with the oldest version that you wish to have in the feedstock (guessing 1.1.2). Then update master to the latest versions (and any other relevant ones inbetween on the way). Then the old support version (guessing 1.1.x) can be a new branch say 1.1 and will just track patch releases for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to only Python 2 in f32eb41.

- nose
- tinyarray
- libgfortran
- libgcc
Copy link
Member

Choose a reason for hiding this comment

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

I suspect we only need one of these. Maybe just libgfortran, but please let me know if there is more going on (e.g. is there C++?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are both needed I think, see this build of this commit 471c711

Copy link
Member

Choose a reason for hiding this comment

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

I see. So it does need C++.

Please add yum_requirements.txt with devtoolset-2-gcc-gfortran. Also please add a selector to use gcc on OS X only. Then please add export LIBRARY_PATH="${PREFIX}/lib" to build.sh. Finally please drop libgcc, but keep/add libgfortran (in build and run).

Copy link
Member

Choose a reason for hiding this comment

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

This strategy is used for scipy, hdf5, and other C/C++/Fortran libraries of note.

xref: conda-forge/scipy-feedstock#2
xref: conda-forge/scipy-feedstock#3

Copy link
Contributor Author

@basnijholt basnijholt Nov 2, 2016

Choose a reason for hiding this comment

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

I did what you suggested, but still getting this error: ImportError: /usr/lib64/libstdc++.so.6: version GLIBCXX_3.4.15' not found (required by /opt/conda/envs/_test/lib/python2.7/site-packages/kwant/graph/slicer.so). I have added export LD_LIBRARY_PATH="${PREFIX}/lib"tobuid.sh`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding LD_LIBRARY_PATH alone didn't help, still the same error message.


build:
skip: true # [win]
skip: true # [py3k]
Copy link
Member

@jakirkham jakirkham Nov 2, 2016

Choose a reason for hiding this comment

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

These can be combined skip: true # [win or py3k].

Also is it that this version is Python 2 only or is it that this version is the last one that supports Python 2, but also supports Python 3? Nevermind answered my own question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 82ea10c. There is no version that combines both Python 2 and 3 simultaneously.

@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/kwant) and found some lint.

Here's what I've got...

For recipes/kwant:

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

- blas 1.1 {{ variant }}
- openblas 0.2.18|0.2.18.*
- mumps
- numpy
Copy link
Member

Choose a reason for hiding this comment

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

Is it linking to numpy? If so, this needs to be numpy x.x.

Copy link
Contributor Author

@basnijholt basnijholt Nov 2, 2016

Choose a reason for hiding this comment

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

Yes, it is linking to numpy. Fixed in e2bb0c2.

- blas 1.1 {{ variant }}
- openblas 0.2.18|0.2.18.*
- mumps
- numpy
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e2bb0c2

@jakirkham
Copy link
Member

Also please add [skip appveyor] to commit messages as this does not build on Windows.

@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/kwant) and found it was in an excellent condition.

@jakirkham
Copy link
Member

So I think you have CircleCI enabled on your fork of staged-recipes. Could you please disable it? Seems to be getting in the way of running it here at staged-recipes.

@basnijholt
Copy link
Contributor Author

I stopped it :-)

@jakirkham
Copy link
Member

Toggling to get CircleCI to restart on staged-recipes ( hopefully 🍀 ).

@jakirkham jakirkham closed this Nov 2, 2016
@jakirkham jakirkham reopened this Nov 2, 2016
@jakirkham
Copy link
Member

Seems to have worked. 😄

@basnijholt
Copy link
Contributor Author

And also the builds are passing 👍

@jakirkham jakirkham merged commit 968efa5 into conda-forge:master Nov 3, 2016
@jakirkham
Copy link
Member

Thanks @basnijholt .

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.

4 participants