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

Lammps #5790

Merged
merged 57 commits into from
May 13, 2018
Merged

Lammps #5790

merged 57 commits into from
May 13, 2018

Conversation

jan-janssen
Copy link
Member

Adding Lammps as requested by @CJ-Wright #5784

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

jan-janssen and others added 2 commits May 4, 2018 08:27
@jan-janssen jan-janssen mentioned this pull request May 4, 2018
- python
- toolchain
- setuptools
- gcc
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 switch this to the new style compilers

build:
  - {{ compiler('c') }}
  - {{ compiler('cxx') }}
  - {{ compiler('fortran') }}

you can remove gcc and toolchain

Copy link
Member Author

Choose a reason for hiding this comment

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

@mariusvniekerk I put them in as otherwise I get the following warnings:

WARNING (lammps,bin/lmp_serial): /lib64/libdl.so.2 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?
   INFO (lammps,bin/lmp_serial): Needed DSO lib/libpython2.7.so.1.0 found in conda-forge::python-2.7.14-5
WARNING (lammps,bin/lmp_serial): /lib64/libc.so.6 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?
WARNING (lammps,bin/lmp_serial): Needed DSO lib/libgfortran.so.3 found in ['libgcc', 'gcc']
WARNING (lammps,bin/lmp_serial): .. but ['libgcc', 'gcc'] not in reqs/run, i.e. it is overlinked (likely) or a missing dependency (less likely)
WARNING (lammps,bin/lmp_serial): Needed DSO lib/libgcc_s.so.1 found in ['libgcc-ng', 'gcc']
WARNING (lammps,bin/lmp_serial): .. but ['libgcc-ng', 'gcc'] not in reqs/run, i.e. it is overlinked (likely) or a missing dependency (less likely)
WARNING (lammps,bin/lmp_serial): Needed DSO lib/libstdc++.so.6 found in ['gcc', 'libstdcxx-ng']
WARNING (lammps,bin/lmp_serial): .. but ['gcc', 'libstdcxx-ng'] not in reqs/run, i.e. it is overlinked (likely) or a missing dependency (less likely)
WARNING (lammps,bin/lmp_serial): Needed DSO lib/libz.so.1 found in ['zlib']
WARNING (lammps,bin/lmp_serial): .. but ['zlib'] not in reqs/run, i.e. it is overlinked (likely) or a missing dependency (less likely)
WARNING (lammps,bin/lmp_serial): /lib64/libpthread.so.0 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?
WARNING (lammps,bin/lmp_serial): /lib64/libutil.so.1 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?
WARNING (lammps,bin/lmp_serial): /lib64/libm.so.6 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): /lib64/libdl.so.2 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): /lib64/libc.so.6 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): Needed DSO lib/libstdc++.so.6 found in ['gcc', 'libstdcxx-ng']
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): .. but ['gcc', 'libstdcxx-ng'] not in reqs/run, i.e. it is overlinked (likely) or a missing dependency (less likely)
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): Needed DSO lib/libgcc_s.so.1 found in ['libgcc-ng', 'gcc']
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): .. but ['libgcc-ng', 'gcc'] not in reqs/run, i.e. it is overlinked (likely) or a missing dependency (less likely)
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): Needed DSO lib/libz.so.1 found in ['zlib']
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): .. but ['zlib'] not in reqs/run, i.e. it is overlinked (likely) or a missing dependency (less likely)
   INFO (lammps,lib/python2.7/site-packages/liblammps.so): Needed DSO lib/libpython2.7.so.1.0 found in conda-forge::python-2.7.14-5
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): Needed DSO lib/libgfortran.so.3 found in ['libgcc', 'gcc']
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): .. but ['libgcc', 'gcc'] not in reqs/run, i.e. it is overlinked (likely) or a missing dependency (less likely)
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): /lib64/libpthread.so.0 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): /lib64/libutil.so.1 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?
WARNING (lammps,lib/python2.7/site-packages/liblammps.so): /lib64/libm.so.6 not found in sysroot, is this binary repackaging? .. do you need to use install_name_tool/patchelf?

Copy link
Member

Choose a reason for hiding this comment

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

What version of conda-build are you using?

Copy link
Member

Choose a reason for hiding this comment

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

@conda-forge/core This seemed to work with toolchain, but not with the new compiler syntax, thoughts?

run:
- python
- mpich
- gcc
Copy link
Member

Choose a reason for hiding this comment

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

Does this need gcc at runtime?

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

Here's what I've got...

For recipes/lammps:

  • The requirements/build section should be defined before the requirements/run section.

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

@isuruf
Copy link
Member

isuruf commented May 4, 2018

lammps's Makefiles don't use CFLAGS, CPPFLAGS and LDFLAGS from the environment, which is really annoying. That's why zlib.h is not found. I suggest raising an issue upstream about using those flags.

@jan-janssen
Copy link
Member Author

@isuruf @CJ-Wright @mariusvniekerk I reverted the recipe back to the old format, as the new format is not working with the current version of Lammps, can one of you approve the merge request? Or is there anything else I can do?

@CJ-Wright
Copy link
Member

LGTM


source:
fn: stable_16Mar2018.tar.gz
url: https://github.com/lammps/lammps/archive/stable_16Mar2018.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

As is, it's really easy to just change version at the top and not realize you're actually building the old version. It's also not very friendly with @regro-cf-autotick-bot as-is.

This way should solve the not-actually-updating issue, and possibly help with bot updates in the future (though that depends on regro/cf-scripts#145):

{% set version = "2018.03.16" %}
{% set date = datetime.date.strptime(version, "%Y.%m.%d") %}
[...]

source:
  fn: {{ name }}-{{ version }}.tar.gz
  url: https://github.com/lammps/lammps/archive/stable_{{ date.day }}{{ "{:%b%Y}".format(date) }}.tar.gz

Unfortunately Python doesn't have a datestring code for a non-zero-padded day, so we have to do the {{ date.day }} thing separately.

(Note that the fn key is confusingly named: it's not the last component of the url, but the filename name that conda should use to cache the source under. You almost always want to leave it as {{ name }}-{{ version }}.tar.gz.)

Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to go the other way with version being in the upstream version format? This way the bot would be happy and provide the correct version. I would make a second variable conda_version which was the version which is parsed from version and is conda complaint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that's easy too:

{% set version = "stable_16Mar2018" %}
{% set date = datetime.datetime.strptime(version.split('_')[1], "%d%b%Y") %}
{% set conda_version = "{:%Y.%m.%d}".format(date) %}

package:
  name: {{ name|lower }}
  version: {{ conda_version }}

source:
  fn: {{ name }}-{{ conda_version }}.tar.gz
  url: https://github.com/lammps/lammps/archive/{{ version }}.tar.gz

Does the bot actually read the current version from the graph from the set version line and not from package/version?

Copy link
Member

Choose a reason for hiding this comment

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

The problem isn't the reading of the version (which it does get from package/version) but what the regex replacement on the jinja variable would do. The replacement sets the version to whatever the upstream gives us back (at least for GitHub urls).

Copy link
Contributor

Choose a reason for hiding this comment

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

But does this mean then that the PRs will be appropriate, but it'll either always or never send them based on how it sorts stable_16Mar2018 against 2018.03.16?

I guess it only sends one PR per version anyway, so it'll be reasonably okay?

Copy link
Member

Choose a reason for hiding this comment

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

The idea is that we can launder the upstream version through the yaml so we can load the properly formatted version for comparison to the current version.

Copy link
Member

Choose a reason for hiding this comment

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

Since that code hasn't been written yet it might be best to blacklist this from the bot for now, that way we don't bombard the maintainers with PRs.

@djsutherland
Copy link
Contributor

Can we use their cmake system instead of make, since their makefiles suck?

Would really prefer to use the compiler syntax if at all possible.

@jan-janssen
Copy link
Member Author

I have to admit I got a little lost in the discussion, is it Oktober accept the recipe in the current version until the official maintainers update the code base?

@djsutherland
Copy link
Contributor

@jan-janssen: Two suggested changes:

  • One is to switch to the versioning scheme Lammps #5790 (comment). This will make updates easier (you only have to change version at the top, instead of remembering to change it in more than one place and getting silently incorrect builds if you forgot one), and hopefully will make life easier for the auto-updating bot though that won't quite work yet.

  • Another is to try using the cmake-based build system so that we can (try to) use the {{ compiler() }} format, because upstream's Makefiles don't respect environment variables that we need. That's the future of conda recipes, and if we can get it that way now, we won't have to change it later. If that doesn't work, fine, but let's try to get that working first.

@jan-janssen
Copy link
Member Author

@dougalsutherland I updated the versioning scheme and tried the cMake based build - but I was not able to get it working e85a995 . It seems to be under development at the moment lammps/lammps#616 - hopefully the next release is able to build using cMake.

@jan-janssen
Copy link
Member Author

@dougalsutherland I did the small fixes in addition to the MPI implementation. The only thing that did not work was moving setuptools from host to build, if I do that the tests fail f9345b0 .

@djsutherland
Copy link
Contributor

Sorry, I guess I wasn't clear in my comment before: I only meant that cmake should be in build. python / setuptools should definitely be in host.

Thanks for making all these changes! Looks great now. 👍

Will send a PR to add the MPI variant matrix after the feedstock is created.

@djsutherland djsutherland merged commit 3e7a58b into conda-forge:master May 13, 2018
@djsutherland
Copy link
Contributor

FYI, we're having issues with converting recipes into feedstocks right now (#5846), so it'll take a bit for this recipe to appear.

@jan-janssen
Copy link
Member Author

@dougalsutherland So it seems that currently lammps is only build with mpich - how can I extend the matrix for the feedstock?

@djsutherland
Copy link
Contributor

djsutherland commented May 15, 2018

Looks like you figured out one way, but I'll send a PR with the new way now. :)

In general, it's a good idea to work on feedstocks by sending PRs from a fork rather than committing straight to master. That way if something goes wrong, it's less likely to release a broken build....

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.

6 participants