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

Jump to bottom

blast: init at 2.10.0 #61430

Merged
merged 10 commits into from Jan 7, 2020
Merged

blast: init at 2.10.0 #61430

merged 10 commits into from Jan 7, 2020

Conversation

luispedro
Copy link
Contributor

Motivation for this change

BLAST is one of the basic bioinformatics packages

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@FRidh
Copy link
Member

FRidh commented May 13, 2019

@GrahamcOfBorg build blast

@FRidh
Copy link
Member

FRidh commented May 14, 2019

The build seems to fail.

@luispedro
Copy link
Contributor Author

My bad, I thought I had built it in a sandbox, but apparently I got fooled by running nix-build with --sandbox, which does not trigger a rebuild if the package was previously built without --sandbox.

Anyway, this package aggressively wants to build using utilities in /bin so I had to patch around it.

@luispedro
Copy link
Contributor Author

@GrahamcOfBorg build blast

@luispedro
Copy link
Contributor Author

I think the issues are fixed now. This is an important package in bioinformatics that'd be great to have in nixpkgs.

@FRidh
Copy link
Member

FRidh commented May 22, 2019

@GrahamcOfBorg build blast

1 similar comment
@FRidh
Copy link
Member

FRidh commented May 22, 2019

@GrahamcOfBorg build blast

@orivej
Copy link
Contributor

orivej commented Jun 13, 2019

See also #33961. (I'm OK with this new PR, it is just more difficult to maintain.)

@luispedro
Copy link
Contributor Author

I wasn't aware of the older PR.

Given the centrality of BLAST in the field and the fact that it's a very stable tool, I think it's best to have something accepted even if it is not always as up to date (for the non-bioinformaticians: it's best to have a 2-year old version of grep that none at all).

I personally don't care if it's the older version in #33961 (although maybe that one should have some of the postInstall steps here) or this one.

@luispedro
Copy link
Contributor Author

Updated against master and it still builds/works cleanly.

@luispedro
Copy link
Contributor Author

Fixed for master (in particular, newer boost versions cause errors) and rebased.

@pschuprikov
Copy link
Contributor

I have successfully built it locally. The tests are not being run

running tests
no check/test target in Makefile, doing nothing

Since you remove the unit tests at postInstall due to the lack of external databases, are you sure thatdoCheck = true is any useful? (BTW, boost seems to be only needed for tests)

@luispedro
Copy link
Contributor Author

You're right. Disabled tests.

@markuskowa
Copy link
Member

markuskowa commented Dec 19, 2019 •
edited

@GrahamcOfBorg build blast

@markuskowa
Copy link
Member

The build fails on darwin, Is it supposed to run on a Mac?

@pschuprikov
Copy link
Contributor

Yes, it is. I'll try to see what's wrong.

@pschuprikov
Copy link
Contributor

The issue seems to be with the missing frameworks. At least, adding ApplicationServices makes it pass the configure step and start building. I'll check the full compilation in the coming days.

luispedro and others added 7 commits January 6, 2020 15:52
@luispedro
blast: init at 2.9.0
71bfa79 
This is widely used in bioinformatics.
@luispedro
blast: init at 2.9.0
d0b28d6
@luispedro
MIN Reduce patching by using substituteInPlace
2c58877
@luispedro
blast: downgrade boost
801aa76 
Newer versions cause errors in the unit_test module
@luispedro
Disable tests
f3a72da 
Most tests require network use and/or locally-installed databases.
@pschuprikov @luispedro
blast: do not build extra deps, +darwin, +dll
490c034
@luispedro
MIN Remove unnecessary shebang patch
2801e96 
The bash patch is automatic
The perl patches become automatic if perl is included in buildInputs

suggested by @markuskowa on GH
@luispedro
Copy link
Contributor Author

Rebased to master & removed the unnecessary shebang patching.

@luispedro
Copy link
Contributor Author

Updated to 2.10.0. It was a simple version+hash update, so that's a good sign.

@markuskowa
Copy link
Member

bin/get_species_taxids.sh fails with:

Cannot find Entrez EDirect esearch tool, please see installation in https://www.ncbi.nlm.nih.gov/books/NBK179288/
results/blast/bin/get_species_taxids.sh: line 1: /bin/rm: No such file or directory

It looks like there are some hard coded /bin/... paths in the script.

@markuskowa markuskowa changed the title Add ncbi blast blast: init at 2.10.0 Jan 6, 2020
@luispedro
Copy link
Contributor Author

Cannot find Entrez EDirect esearch

Ah, yes, that is a tool that must be installed separately. Currently, there is no nix package for it, though.

It looks like there are some hard coded /bin/... paths in the script.

Aargh, this package really does want to use /bin/rm all the time for no good reason.

They also set $PATH to include some internal paths (`export PATH=/bin:/usr/bin:/am/ncbiapdata/bin:$HOME/edirect:$PATH).

@markuskowa
Copy link
Member

@luispedro Do you want to fix the /bin/rm entries (e.g. with a substituteInPlace) so that it works out of the box later or leave it for now?

@luispedro
Copy link
Contributor Author

Fixed the /bin/rm dependency, so that, if users have entrez installed somehow, it will be useful.

@markuskowa
Copy link
Member

@luispedro there's still a problem with the darwin build.

@luispedro
Copy link
Contributor Author

Unfortunately, I have no way of testing on Darwin. I can only restrict the package to Linux if nobody else can work on it.

@markuskowa
Copy link
Member

Restricting the package to linux seems to be the best option for now. If someone else is willing to fix it we can always do it later in a separate PR.

@luispedro
Copy link
Contributor Author

Restricted to Linux now.

@pschuprikov
Copy link
Contributor

I do plan to fix the build for Darwin, but it can definitely be done as a separate PR.

@markuskowa markuskowa merged commit fdfebaf into NixOS:master Jan 7, 2020
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Jan 7, 2020
@luispedro @dtzWill
blast: init at 2.10.0 (NixOS#61430)
389a431 
Co-authored-by: Pavel Chuprikov <pschuprikov@gmail.com>
(cherry picked from commit fdfebaf)
@luispedro luispedro deleted the add_NCBI_blast branch January 8, 2020 04:04
@luispedro luispedro mentioned this pull request Jan 8, 2020
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pschuprikov pschuprikov pschuprikov left review comments

@c0bw3b c0bw3b c0bw3b left review comments

@markuskowa markuskowa markuskowa left review comments

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@luispedro @FRidh @orivej @pschuprikov @markuskowa @c0bw3b