-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
ADIOS2 Feedstock #8526
ADIOS2 Feedstock #8526
Conversation
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 ( |
a618cf3
to
1849f63
Compare
e87f43d
to
faf4b1b
Compare
@chuckatkins in coordination with @williamfgc, I have adopted your conda scripts for conda-forge. There are still some smaller hiccups, but at least on Linux and OSX the build passes already. Both of you, please confirm that you want to be listed as co-maintainers of the feedstock and please tell me anyone you want to add as well. |
Prepare a feedstock for ADIOS2, including MPI variants.
I agree to be listed as co-maintainer of this feedstock |
@chuckatkins thanks! @williamfgc it will be
|
I'll move Windows HDF5 support to an in-feedstock update since I cannot spot the linker error here: https://ci.appveyor.com/project/conda-forge/staged-recipes/builds/25643672 |
1a1eef6
to
a7f0d49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few cosmetic things then LGTM 👍
@@ -0,0 +1,201 @@ | |||
Apache License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this file? I think it should be in the source tarball
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we should always add the LICENSE file separately?
(link in PR description template)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always need to package it but we prefer to get it from source tarball(s) instead of including it in the recipe directory if we can to ensure it's correct and up to date.
Clean up: suggestions by Chris (thanks!) Co-Authored-By: Chris Burr <[email protected]>
Thanks for the review @chrisburr, I added your suggestions and CI passed again :) |
ping @chrisburr :) |
recipes/adios2/build.sh
Outdated
CMAKE_PLATFORM_FLAGS+=(-DADIOS2_USE_BZip2=OFF) | ||
elif [[ ${target_platform} =~ linux.* ]]; then | ||
# link transitive libraries during build of intermediate wrapper lib | ||
# export LDFLAGS="${LDFLAGS} -Wl,-rpath-link,${PREFIX}/lib" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these comments still needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Luckily not, will remove.
# prioritize nompi variant via build number | ||
{% if mpi == 'nompi' %} | ||
{% set build = build + 100 %} | ||
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like the right way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not? That's how @minrk showed it to me (hdf5, h5py et al.). We increase the build number for nompi variants to select them by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? That is kinda nuts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have documentation for this do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat, I am just following recently explored best practices:
- https://hackmd.io/ry4uI0thTs2q_b4mAQd_qg
- MPI Variants adios-feedstock#12 (comment)
- https://github.com/conda-forge/hdf5-feedstock/blob/master/recipe/meta.yaml
- https://github.com/conda-forge/h5py-feedstock/blob/master/recipe/meta.yaml
Bumping the build number with an offset actually sounds quite legit to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this way of doing things is causing issues in the solver with multi output recipes that depend on packages with MPI variants. I've not managed to make sense of it yet but these builds show it:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have documentation for this do we?
It's in the docs
Build offset is the only way to indicate preference in the solver that I'm aware of that works for existing packages. Note that the original and first-implemented plan was to use track_features to deprioritize, but this does not work for packages that have ever been published before, but I think it might work for new packages, or packages where all previous builds have been fully revoked or hotfixed.
In most cases, packages that require MPI express no preference, but it's more complicated when you have three variants and want users to opt-in to MPI (e.g. hdf5). Conda still has no ability to express preferences in a natural way, so any solution is going to be a bit wonky until there's a real solution. For that reason, I might recommend avoiding a 'preference' unless it's important to avoid picking up a variant in certain circumstances, at least until there's a better way.
5e48a18
to
d654b04
Compare
Prepare a feedstock for ADIOS2, including MPI variants.
We will enable aarch64 and ppc64le support later on in the feedstock.
Checklist