cschin / peregrine-2021

Other
58 stars 6 forks source link

Let's get this on Bioconda! #1

Closed rpetit3 closed 2 years ago

rpetit3 commented 2 years ago

Hello!

Are you interested in working together to get this onto Bioconda?

Cheers and great job! Robert

cschin commented 2 years ago

I have not built a bioconda package myself yet. (I checked it before but since others can work on it so I passed it :) ), yes. if you think the license is compatible, I am happy that this is part of bioconda.

rpetit3 commented 2 years ago

Great! License should be fine.

For starters could you create a release? I notice there's a tag, but not a release. Once a release is available I'll get the recipe started. Might just bug you about dependencies

rpetit3 commented 2 years ago

PR submitted (https://github.com/bioconda/bioconda-recipes/pull/32806), I think I got all the dependencies. I'm probably going to clean it up to determine which ones are needed.

Please let me know if I am missing any in here: https://github.com/bioconda/bioconda-recipes/blob/rp3-add-peregrine-2021/recipes/peregrine-2021/meta.yaml

cschin commented 2 years ago

Thank you. I was actually trying to figure out how to build Rust code for bioconda. Getting a bit closer looking at other examples, but I am not there yet. I will look over the dependence. The current build depends on a couple my own GitHub repo that fix a couple of other packages that I use. I hope that is ok.

cschin commented 2 years ago

@rpetit3 I tried to build it locally with the boostrap.py + bioconda-utils method, I still run into a couple of issues. Let me make sure it builds before we commit the receipt.

cschin commented 2 years ago

@rpetit3 I made a new PR https://github.com/bioconda/bioconda-recipes/pull/32810 This commit https://github.com/cschin/peregrine-2021/commit/2ffbb9b84d75f6c018fad0321c71dbffbedc978d addresses the problem for building for bioconde. The original build.rs requires the source directory as a git-controlled repo. the release tarball from GitHub does not contain .git file so the build would fail without changing the build.rs.

cschin commented 2 years ago

According to this https://dev.azure.com/bioconda/bioconda-recipes/_build/results?buildId=10917&view=results, the Linux build works, but MacOS build fails as the RUSAGE_THREAD used for monitoring resource usage is Linux specific. To fix this we need to add conditional compile flags for MacOS.

rpetit3 commented 2 years ago

Do you think just setting it to RUSAGE_THREAD=0 would do the trick? (I'm not to familiar with Rust)

cschin commented 2 years ago

It is a RUST symbol. RUST is quite strict about this kind of thing. I need to check if I can replace it with RUSAGE_SELF. The code is only for logging propose. I actually have never even attempted to compile it on MacOS X so I did not catch that before. (I did a lot on AARCH64 more.)

rpetit3 commented 2 years ago

I found this recipe with OSX related changes for a run install, but it looks unrelated

https://github.com/bioconda/bioconda-recipes/blob/master/recipes/fmlrc2/build.sh

cschin commented 2 years ago

to do it properly (as getting the information of per-thread CPU time on MacOS) is trickier.

  1. change to RUSAGE_SELF (equivalent to set RUSAGE_THREAD=0, RUSAGE_SELF is 0) does not give the proper measurement. It overestimated the time used. I think it includes the parent thread time.

  2. The solution from this stack flow post might work for macOS: https://stackoverflow.com/questions/5652463/equivalent-to-rusage-thread-darwin But we might need to provide some C code for it, and we have to test it as well.

I think it is probably good to just turn off the CPU time usage measurement when compiling for MacOS for now. Once we get time for this and there are more people using it in macOS, we can add it to macOS build with proper tests.

cschin commented 2 years ago

@rpetit3

This https://github.com/bioconda/bioconda-recipes/pull/32810/commits/ad7a0ec18177f776ecc5d997ebf3279e00911629 for this PR https://github.com/bioconda/bioconda-recipes/pull/32810 goes through the CI.

ref: https://dev.azure.com/bioconda/bioconda-recipes/_build/results?buildId=11016&view=results

Please check, if it works, we can close this issue. Thanks.

rpetit3 commented 2 years ago

Looked good to me! Glad we were able to get it on Bioconda!

rpetit3 commented 2 years ago

Incase you did not know, now whenever you submit a new release the Bioconda Bot will find it and create a new PR automatically

cschin commented 2 years ago

cool. that helps a bit for CI which I don't have a chance to set up yet. Eventually, I need to set up a more proper regression test. I have some ad hoc testing processes but I still need to take some time to set it up for automation.

rpetit3 commented 2 years ago

feel free to reach out about GitHub Actions.

Here's a few examples from mine:

https://github.com/bactopia/bactopia/tree/master/.github/workflows https://github.com/rpetit3/dragonflye/tree/main/.github/workflows

The bactopia one is using pytest and pytest-workflows for the testing

cschin commented 2 years ago

I pushed a new release and the bioconda bot did pick up and try to rebuild a package for the new release. However, the OS X build failed. This time it is a bit complicated. Compiling one of the memory allocator (mimalloc) library was the issue. Interestingly, it worked last time (v.0.4.12).

The issue is that one of the C code had an unknown symbol (https://dev.azure.com/bioconda/25074ded-3133-43ca-af80-895ab87b53cb/_apis/build/builds/11943/logs/37)

2022-02-15T17:58:57.7382850Z 17:58:57 BIOCONDA INFO (ERR)   cargo:warning=In file included from c_src/mimalloc/src/random.c:198:
2022-02-15T17:58:57.7384410Z 17:58:57 BIOCONDA INFO (ERR)   cargo:warning=/Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.13.sdk/usr/include/CommonCrypto/CommonRandom.h:35:9: error: unknown type name 'CCCryptorStatus'

According to a couple of online documents mentioning the related issues (e.g. see https://github.com/koka-lang/koka/issues/243 ) It seems the it may need to add this to mimalloc/src/random.c to solve this problem:

# include <CommonCrypto/CommonCryptoError.h>

@rpetit3 what do you think the better way to make the build go through? It is a bit strange that we did not see it last time.

I am not sure what the OSX version used in the building machine may affect this. I would suggest retry to build it again. If it still fail, we need to figure a patch mechanism for OS X build. I can remove the mimalloc dependence for OS X but I hope that is the last resort.

rpetit3 commented 2 years ago

Looks like a fix was applied and the latest version was merged

https://github.com/bioconda/bioconda-recipes/pull/33185

cschin commented 2 years ago

nice. Thanks. I will modify the Cargo.toml in the next release. closing.