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

Add igphyml #18945

Merged
merged 14 commits into from
Nov 27, 2019
Merged

Add igphyml #18945

merged 14 commits into from
Nov 27, 2019

Conversation

dpryan79
Copy link
Contributor

@dpryan79 dpryan79 commented Nov 27, 2019

Bioconda requires reviews prior to merging pull-requests (PRs). To facilitate this, once your PR is passing tests and ready to be merged, please add the please review & merge label so other members of the bioconda community can have a look at your PR and either make suggestions or merge it. Note that if you are not already a member of the bioconda project (meaning that you can't add this label), please ping @bioconda/core so that your PR can be reviewed and merged (please note if you'd like to be added to the bioconda project). Please see #15332 for more details.

  • 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).

@dpryan79
Copy link
Contributor Author

Xref: #18930

@dpryan79
Copy link
Contributor Author

@bioconda-bot please fetch artifacts

@BiocondaBot
Copy link
Collaborator

Package(s) built on CircleCI are ready for inspection:

Arch Package Repodata
linux-64 igphyml-1.0.7-h516909a_0.tar.bz2 repodata.json
osx-64 igphyml-1.0.7-h01d97ff_0.tar.bz2 repodata.json

You may also use conda to install these:

  • For packages on linux-64:
conda install -c https://84274-42372094-gh.circle-artifacts.com/0/tmp/artifacts/packages <package name>
  • For packages on osx-64:
conda install -c https://84278-42372094-gh.circle-artifacts.com/0/tmp/artifacts/packages <package name>

Docker image(s) built:

Package Tag Install with docker
igphyml 1.0.7--h516909a_0
showcurl "https://84274-42372094-gh.circle-artifacts.com/0/tmp/artifacts/images/igphyml%3A1.0.7--h516909a_0.tar.gz" | gzip -dc | docker load

@dpryan79
Copy link
Contributor Author

@PertuyF Can you see if this works? I'm not entirely sure how this program will work since it really does hardcode the exact path to the motifs in the source code. I've tried making that relative to where the binary is, but I'm suspecting that that won't work and that the source code will have to get modified. That's probably also the only way to make relocatable binaries like would be needed for a conda install.

@dpryan79 dpryan79 changed the title Add igphyml Add igphyml (do not merge) Nov 27, 2019
@PertuyF
Copy link
Contributor

PertuyF commented Nov 27, 2019

@dpryan79 binary works OK, but we're missing the IGPHYML_PATH variable that should point to $PREFIX/share/igphyml/motifs/ to pass the tests.

And for Linux version I get a warning when setting --threads that binary was not compiled with OpenMP. And I can confirm the behavior is different with a locally built omp-enabled version.

Unfortunately I can't access a machine to test OSX version.

@dpryan79
Copy link
Contributor Author

I'm amazed that works at all. I guess this needs an activation script. The OMP thing just needs a new -D flag I suspect.

dpryan79 and others added 8 commits November 27, 2019 14:25
* Use sequences from example.fasta
* Use minimum number of 3 entries
* Include root sequence ID from doc examples for consistency
* Test binary is built with OpenMP
* Test hotspot tables are found with `IGPHYML_PATH` env var
@PertuyF
Copy link
Contributor

PertuyF commented Nov 27, 2019

For some reason my test file wasn't included for final test phase. Although I think I followed the doc.
Anyway, it is likely better to include the original examples dir (268KB) for users to follow the doc examples.

Can be OK to use these files to run tests, but I can also include my test.fasta in ${PREFIX}/share/igphyml/test/ and save us a few minutes of computation...

@PertuyF
Copy link
Contributor

PertuyF commented Nov 27, 2019

There is something wrong with tests during mulled-build. Do you have a clue @dpryan79 ?

@dpryan79
Copy link
Contributor Author

Test files don't make it into the container for testing, since they're not part of the end package (just the recipe). For the mulled tests there's just a binary being called, so the activation scripts are never run.

@PertuyF
Copy link
Contributor

PertuyF commented Nov 27, 2019

Good job, this looks cleaner!
Just noticed I missed last version, so I'll bump to 1.1.0
After that we can close my initial PR and merge this one.

Next step will be Blas support :)

@PertuyF PertuyF mentioned this pull request Nov 27, 2019
5 tasks
@dpryan79 dpryan79 changed the title Add igphyml (do not merge) Add igphyml Nov 27, 2019
@dpryan79
Copy link
Contributor Author

@PertuyF Go ahead and approve this PR and then merge it with @bioconda-bot please merge whenever you're ready.

@PertuyF
Copy link
Contributor

PertuyF commented Nov 27, 2019

@bioconda-bot please merge

@BiocondaBot
Copy link
Collaborator

I will attempt to upload artifacts and merge this PR. This may take some time, please have patience.

@BiocondaBot
Copy link
Collaborator

I received an error uploading the build artifacts or merging the PR!

@dpryan79
Copy link
Contributor Author

Meh, normal merge it is then.

@dpryan79 dpryan79 merged commit 5297d57 into master Nov 27, 2019
@dpryan79 dpryan79 deleted the add_igphyml branch November 27, 2019 21:36
@dpryan79
Copy link
Contributor Author

It seems there are some networking issues that borked the bot's upload. We'll just use the regular CI then to upload things.

@PertuyF
Copy link
Contributor

PertuyF commented Nov 27, 2019

Good for me, as long as the package is released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants