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

samtools: 1.5.0 -> 1.6.0 #29963

Merged
merged 6 commits into from Oct 2, 2017
Merged

samtools: 1.5.0 -> 1.6.0 #29963

merged 6 commits into from Oct 2, 2017

Conversation

mimame
Copy link
Member

@mimame mimame commented Sep 30, 2017

Motivation for this change

htslib, samtools and bcftools

  • Update to the last versions
  • Prograte htslib to samtools and bcftools
  • Enable enableParallelBuilding
  • Do tests
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@orivej
Copy link
Contributor

orivej commented Sep 30, 2017

Using binaries from PATH is the reasonable thing to do for upstream projects, so I don't think that we should patch in absolute paths. Just add the necessary programs to nativeBuildInputs.

@mimame
Copy link
Member Author

mimame commented Sep 30, 2017

Thanks a lot @orivej. I didn't do that because I didn't know which was better buildInputs vs nativeBuildInputs. I will update the commits as soon as possible

@mimame
Copy link
Member Author

mimame commented Sep 30, 2017

@orivej Fixed. Don't hesitate to ask for more changes

@orivej
Copy link
Contributor

orivej commented Sep 30, 2017

Why didn't you add perl too? Note that /usr/bin/env is fine and should be left as is.

@mimame
Copy link
Member Author

mimame commented Sep 30, 2017

@orivej, thanks a lot for your support and help
I hope to have done all right this time. I've also prograted htslib to samtools and bcftools.

@orivej
Copy link
Contributor

orivej commented Oct 1, 2017

Please review my changes.

samtools and bcftools install some python and perl scripts. We do not even depend on python, and I have not checked that perl scripts have all the necessary dependencies to run. This can be left as is until someone needs them.

@mimame
Copy link
Member Author

mimame commented Oct 1, 2017

That's perfect but I think that htslib has to be propagated to samtools and bcftools for using bgzip and tabix

@orivej
Copy link
Contributor

orivej commented Oct 1, 2017

Do you mean that users of samtools and bcftools also need bgzip and tabix in their PATH?

@mimame
Copy link
Member Author

mimame commented Oct 1, 2017

I think so because in other distros the htslib dependency is visible from PATH by default.
Which is your opinion?

@orivej
Copy link
Contributor

orivej commented Oct 2, 2017

Other distros have no other way. From a quick glance it does not seem that samtools need htslib binaries. (Even if they did, the proper solution is a program wrapper that adds htslib to the PATH of the wrapped program, not a propagation of htslib.)

@orivej orivej merged commit 047c576 into NixOS:master Oct 2, 2017
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.

None yet

2 participants