-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
Add benchmark support in shellFor #102323
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
Conversation
Benchmark dependencies are not included by shellFor unless we pass `doBenchmark = true` when generating the derivation. This commit passes doBenchmark argument from shellFor to the derivation so that benchmark dependencies get included.
@harendra-kumar This seems reasonable. Do you have some sort of example to prove that this is working as expected? |
@cdepillabout yes, I have been using it for a while to create a nix-shell for streamly for running the benchmarks, see this PR for the nix expression in use. My initial solution was more complex, but I gave it some soak time and it emerged to be simpler. |
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've stared at this shellFor
code a while, and I've come to the conclusion that I don't think the change in this PR is the proper solution.
I think instead what you need to do is enable doBenchmark
on the packages you want benchmark dependencies for. In the example in the comment on the shellFor
function, it would be p.frontend
, p.backend
, and p.common
.
Is there some reason this isn't working?
It does not work. I just tried it and pushed it in this commit composewell/streamly@890d966 . You can pull the repo, change branch to "nix" and try the instructions in the commit message to give it a try. |
It took me quite a few hours, but I finally picked apart @harendra-kumar It turns out you were right. I commented about fixing this in #102598 (comment). |
@cdepillabout thanks for working on this. I am closing this in favor of #102683 . |
Motivation for this change
Benchmark dependencies are not included by shellFor unless we pass
doBenchmark = true
when generating the derivation. This commitpasses doBenchmark argument from shellFor to the derivation so that
benchmark dependencies get included.
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)