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
bioconductor packages: 3.8 -> 3.10, update R packages, other fixes #85522
Conversation
Why do you make a new PR instead of just updating the old one? |
I ran into trouble updating the feature branch on my end. Also did not seem like the original one received much notice so I didn’t think anyone would care. Lmk if you have any suggestion. Happy to take advice. |
(lib.optionals stdenv.isDarwin [ libiconv flock which ]) | ||
++ [ pkgs.gsl liblapack gmp.dev gtk2.dev bzip2.dev pkgs.pkgconfig | ||
lzma icu.dev pcre.dev zlib.dev curl.dev libpng libtiff libjpeg | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will increase the closure size quite a bit for packages that don't need this dependencies.
I'm assuming this is an optimization for your time since it is hard to track down who needs what, but for including this in nixpkgs I think it would be smart to add it on per package basis. I'm no expert in R, but I would assume this would apply also to rPackages. But I understand it adds much more work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I thought about that. If you add these it means a new R packages is more likely to just work. However, what you say about closures is true. A mitigating factor is that the target audience for R mainly wants a development environment with many packages. In that scenario you only pay the size price once. If you think certain packages should be removed from this list I would be willing to add them explicitly as needed. However I don't thing the list should be totally empty. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A mitigating factor is that the target audience for R mainly wants a development environment with many packages. In that scenario you only pay the size price once.
I don't believe that you can legitimately speak for "the target audience" unless you have polled them first in some significant way. As it is, we don't know other people want. We can only speak about what we want.
Personally, I want accurate dependencies. I don't want packages to depend on stuff that they actually don't depend on. Furthermore, this is a guiding principle we have applied all over Nixpkgs, not just for R. This change goes against that design goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current branch there are packages already added to every R derivation, whether they are needed or not. The question is what is the criteria for adding those? I am sure I added some that are reasonable, but perhaps some should be done per-package. What's the right decision process? Should it include likelihood/ease of adding a future R package to nixpkgs? If not, we could make all dependencies explicit. More accurate, smaller closure size. But more breakage, more effort now and more in the future. I honestly don't know the correct answer here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the target audience, it might be the case that the nix/R user only runs batch jobs and need only a few R packages at a time. That would be a fair comment. On the other hand, I can confidently say that there are few if any data scientists using nix/R to build and maintain a dev environment that includes bioconductor packages. That's what prompted me to attempt to improve the situation. Even if this PR got merged, serious problems remain for source packages that update frequently. We don't need a poll to see how conda supports those users better right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current branch there are packages already added to every R derivation, whether they are needed or not.
Which ones are you referring to?
On the target audience, it might be the case that the nix/R user only runs batch jobs and need only a few R packages at a time.
Nobody said that. I all said was that I don't like it when people try to justify technical decisions with the needs of some "silent majority" without any evidence that they have actual knowledge about the needs of that group. I'd rather you speak for what you need than about what some mystical "target audience" needs.
What kind of trouble? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes too many things at once. I would really love to have this submission broken up into separate PRs that are easier to review.
@@ -0,0 +1,27 @@ | |||
#!/bin/sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a test script to build packages. I used it to test all of the packages. Complements the generation of all the packages created by generate-r-packages.R. Intended for use by someone updating all the R packages, not a user trying to install some.
_self = { inherit buildRPackage; } // | ||
import ./bioc-packages.nix { inherit self; derive = deriveBioc; } // | ||
import ./bioc-packages.nix { inherit self; derive = deriveBioc; } // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't mix indentation changes into a PR that changes functionality. This makes the diffs unnecessarily long and hard to read. If you want to improve the formatting, please use a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that looks like a mistake.
}; | ||
|
||
packagesWithNativeBuildInputs = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Please make such a stylistic change in a separate PR. As it is, it's hard hard to tell what you actually changed because the diff is so long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure the diff could be improved and welcome suggestions. But the diff long mainly because it fixes hundreds of packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the diff is unnecessarily long because you've replaced every explicit pkgs
path with an implicit with pkgs;
scope at the top of the attribute. There might be lots of build fixes included in the change you've submitted, but they aren't recognizable because of this noise.
@peti I would attempt to meet the requested changes if the target was clearer. Simply saying to break up the PR doesn't tell me what would make the PR acceptable. |
|
The patch Rhdf5lib.patch also fails for me. And I wholeheartedly agree with @timsears with the need of rapid adaptation for data analysis environments in which packages are changing their dependencies very other day. |
@timsears said "would attempt to meet the requested changes if the target was clearer". Then I went out of my way to say what changes are required in #85522 (comment), but the effect has been zilch. What a waste of time. :-( I'm closing this PR for now. If anyone wants to pick up the baton, please don't hesitate to submit your changes in a new PR. |
@peti It's fine to close this PR. For a new person on nixpkgs I feel your attitude is discouraging at best and borderline toxic. It's not going to be easy to attract new nixpkgs committers without a change of approach and maybe documentation. I truly hope this is not a common experience for others. On the technical side I am going to start over and start by thinking about about the source caching issue, which I uncovered only by attempting this PR and which it does not address. |
@timsears, this whole interaction would be so much more constructive if you could cut out the ad hominem attacks and instead address the technical issues I pointed out about your PR. |
Motivation for this change
Updates cran and bioconductor packages.
Adds bioconductor workflows
Add sample shell environment
Fixes hundreds of existing packages
Marks dozens broken packages as broken, adding hints for next steps to fix them
Fixes many R packages on darwin.
Adds selected packages to the generic builder so that many future R packages will be able to build without customization in nixpkgs.
Adds scripts to help generate lists of selected package names to ease testing.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)