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

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

@luispedro luispedro commented May 13, 2019

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 FRidh commented May 13, 2019

@GrahamcOfBorg build blast

@FRidh
Copy link
Member

@FRidh FRidh commented May 14, 2019

The build seems to fail.

@luispedro luispedro force-pushed the luispedro:add_NCBI_blast branch from f1e40f6 to cc65c7a May 16, 2019
@luispedro
Copy link
Contributor Author

@luispedro luispedro commented May 16, 2019

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

@luispedro luispedro commented May 16, 2019

@GrahamcOfBorg build blast

@luispedro luispedro force-pushed the luispedro:add_NCBI_blast branch from cc65c7a to fcf679c May 17, 2019
@luispedro
Copy link
Contributor Author

@luispedro luispedro commented May 22, 2019

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 FRidh commented May 22, 2019

@GrahamcOfBorg build blast

1 similar comment
@FRidh
Copy link
Member

@FRidh FRidh commented May 22, 2019

@GrahamcOfBorg build blast

@orivej
Copy link
Contributor

@orivej orivej commented Jun 13, 2019

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

@luispedro luispedro force-pushed the luispedro:add_NCBI_blast branch from e13795a to 065585b Jun 14, 2019
@luispedro
Copy link
Contributor Author

@luispedro luispedro commented Jun 14, 2019

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 luispedro force-pushed the luispedro:add_NCBI_blast branch from 065585b to d36923b Aug 5, 2019
@luispedro
Copy link
Contributor Author

@luispedro luispedro commented Aug 7, 2019

Updated against master and it still builds/works cleanly.

@luispedro luispedro force-pushed the luispedro:add_NCBI_blast branch from d36923b to a710cdf Aug 18, 2019
@luispedro luispedro force-pushed the luispedro:add_NCBI_blast branch from a710cdf to 5d35b9c Dec 9, 2019
@luispedro
Copy link
Contributor Author

@luispedro luispedro commented Dec 9, 2019

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

@pschuprikov
Copy link
Contributor

@pschuprikov pschuprikov commented Dec 17, 2019

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

@luispedro luispedro commented Dec 19, 2019

You're right. Disabled tests.

@markuskowa
Copy link
Member

@markuskowa markuskowa commented Dec 19, 2019

@GrahamcOfBorg build blast

@markuskowa
Copy link
Member

@markuskowa markuskowa commented Dec 19, 2019

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

@pschuprikov
Copy link
Contributor

@pschuprikov pschuprikov commented Dec 20, 2019

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

@pschuprikov
Copy link
Contributor

@pschuprikov pschuprikov commented Dec 23, 2019

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 May 10, 2019
This is widely used in bioinformatics.
Newer versions cause errors in the unit_test module
Most tests require network use and/or locally-installed databases.
The bash patch is automatic
The perl patches become automatic if perl is included in buildInputs

suggested by @markuskowa on GH
@luispedro luispedro force-pushed the luispedro:add_NCBI_blast branch from 9362ef4 to 2801e96 Jan 6, 2020
@luispedro
Copy link
Contributor Author

@luispedro luispedro commented Jan 6, 2020

Rebased to master & removed the unnecessary shebang patching.

@luispedro
Copy link
Contributor Author

@luispedro luispedro commented Jan 6, 2020

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

@markuskowa
Copy link
Member

@markuskowa markuskowa commented Jan 6, 2020

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

@luispedro luispedro commented Jan 6, 2020

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

@markuskowa markuskowa commented Jan 6, 2020

@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

@luispedro luispedro commented Jan 7, 2020

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

@markuskowa
Copy link
Member

@markuskowa markuskowa commented Jan 7, 2020

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

@luispedro
Copy link
Contributor Author

@luispedro luispedro commented Jan 7, 2020

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

@markuskowa markuskowa commented Jan 7, 2020

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

@luispedro luispedro commented Jan 7, 2020

Restricted to Linux now.

@pschuprikov
Copy link
Contributor

@pschuprikov pschuprikov commented Jan 7, 2020

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
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
blast on aarch64-linux Success
Details
blast on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Jan 7, 2020
Co-authored-by: Pavel Chuprikov <pschuprikov@gmail.com>
(cherry picked from commit fdfebaf)
@luispedro luispedro deleted the luispedro:add_NCBI_blast branch Jan 8, 2020
@luispedro luispedro mentioned this pull request Jan 8, 2020
4 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.