-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Adds recipe for mccortex #10860
Adds recipe for mccortex #10860
Conversation
winni2k
commented
Sep 7, 2018
- I have read the guidelines for bioconda recipes.
- This PR adds a new recipe.
- AFAIK, this recipe is directly relevant to the biological sciences (otherwise, please submit to the more general purpose conda-forge channel).
- This PR updates an existing recipe.
- This PR does something else (explain below).
@bioconda/core: I think this PR is ready to merge. |
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.
Nice!
- {{ compiler('cxx') }} | ||
host: | ||
- autoconf | ||
- automake |
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.
Build tools go into build:
, so autoconf
and automake
can go there. Do you even need them?
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.
Will do, but see comment below.
|
||
export CFLAGS="-I$PREFIX/include" | ||
export LDFLAGS="-L$PREFIX/lib" | ||
export CPATH=${PREFIX}/include |
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.
Just optics, but write CPATH="$PREFIX/include"
for readability
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.
Sure! But someone should probably fix the docs where I copied this from: https://bioconda.github.io/troubleshooting.html#zlib-errors
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.
Agreed. See bioconda/bioconda.github.io#22
for make_file in libs/string_buffer/Makefile $(find libs/seq_file -name Makefile) $(find libs/seq-align -name Makefile) Makefile; do | ||
sed -i.bak 's/-lz/-lz $(LDFLAGS)/' $make_file | ||
done | ||
|
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.
Think you could make a PR upstream and reference it here? No need to delay the package, but feeding these things upstream would be nice.
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.
Wait ... if this tool uses autoconf/autotools, why do you need to patch the Makefiles?
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.
Mccortex has extensive dependencies that are all part of the source code release. The patching is applied to only a small number of all the make files in the mccortex source code. I need autoreconf for other make files...
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.
Happy to fix the other issues in an upstream PR.
Thanks @winni2k |
* Add mccortex * Refactor meta.yaml * Refactor meta.yaml * Add libtool build dependency * Maybe autoconf is the right library * Fix build number * Also need automake * fix sed issue in Mac OS X * Remove unneeded LIBRARY_PATH in build.sh * Fix minor comments from #10860 * bumb build number