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

criterion: init at 2.3.3 #70403

Merged
merged 3 commits into from Nov 7, 2019
Merged

criterion: init at 2.3.3 #70403

merged 3 commits into from Nov 7, 2019

Conversation

@Yumasi
Copy link
Contributor

Yumasi commented Oct 4, 2019

Motivation for this change

Criterion was not packaged for NixOS, so here is a derivation for it. Alongside it is a derivation for BoxFort, which is a library specifically created to implement Criterion. Since BoxFort is a very nice project, it does not have any release and is built by Criterion's build system against BoxFort's master branch. Since this behavior is not Nix-friendly, I packaged it using the commit hash as a version number. This also guarantees me to have a working Criterion derivation. This PR also adds me in the maintainer list for those 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 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 nix-review --run "nix-review wip"
  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@Yumasi Yumasi changed the title Criterion criterion: init at 2.3.3 Oct 4, 2019
@wucke13

This comment has been minimized.

Copy link
Contributor

wucke13 commented Oct 9, 2019

I guess you forgot to add your package to pkgs/top-level/all-packages.nix? On reviewing your PR
(nix-review pr 70403) the result is that no packages have been build. Please look into this :)

@Yumasi Yumasi force-pushed the Yumasi:criterion branch from 3d1c05a to ce433af Oct 9, 2019
@Yumasi

This comment has been minimized.

Copy link
Contributor Author

Yumasi commented Oct 9, 2019

@wucke13 Thanks for your review! I've pushed updated commits with your comments taken into account.

@Thesola10

This comment has been minimized.

Copy link
Contributor

Thesola10 commented Oct 9, 2019

Hello, I am the author of #70609 which also adds criterion to Nix packages. Since our PRs are conflicting, I wanted to ask if you are okay with my PR, or if I should close it and have yours be merged.

@Yumasi

This comment has been minimized.

Copy link
Contributor Author

Yumasi commented Oct 9, 2019

Hi ! I guess its a matter of who gets to maintain it. You seem to have two extra commits in your pull-request and, purely subjectively, I find my derivation to be cleaner, so I'd say my PR is currently more ready for merge.

@Thesola10

This comment has been minimized.

Copy link
Contributor

Thesola10 commented Oct 9, 2019

I think I actually agree with you on this, I was just working on rebasing these two (accidental) commits away, but your derivation file is more complete and cleaner. (though I can't help but wonder if running unit tests in a package build is really necessary)

I will be closing my PR shortly.

@wucke13

This comment has been minimized.

Copy link
Contributor

wucke13 commented Oct 9, 2019

@Thesola10 @Yumasi

I guess its a matter of who gets to maintain it

Noo! If you want to, add both of you to the maintainer list. It is never wrong to have more people to fix things once they go bad!

@Yumasi

This comment has been minimized.

Copy link
Contributor Author

Yumasi commented Oct 9, 2019

Noo! If you want to, add both of you to the maintainer list. It is never wrong to have more people to fix things once they go bad!

I couldn't agree more! @Thesola10 do you want me to add you to the maintainer list ?

@Thesola10

This comment has been minimized.

Copy link
Contributor

Thesola10 commented Oct 9, 2019

@Yumasi please do it then (my maintainer entry already exists and is thesola10) because I don't want to mess up this PR branch. I don't even think I have access to this patch repo. You can add me to both criterion and boxfort if you want

Yumasi added 3 commits Oct 9, 2019
Signed-off-by: Guillaume Pagnoux <gpagnoux@gmail.com>
Signed-off-by: Guillaume Pagnoux <gpagnoux@gmail.com>
Signed-off-by: Guillaume Pagnoux <gpagnoux@gmail.com>
@Yumasi Yumasi force-pushed the Yumasi:criterion branch from ce433af to b76cfaf Oct 9, 2019
@ofborg ofborg bot requested a review from Thesola10 Oct 9, 2019
@Thesola10

This comment has been minimized.

Copy link
Contributor

Thesola10 commented Oct 10, 2019

that's cool, but why exactly did ofborg request me for review?

Copy link
Contributor

Thesola10 left a comment

Looks good to me.

@Yumasi

This comment has been minimized.

Copy link
Contributor Author

Yumasi commented Oct 10, 2019

that's cool, but why exactly did ofborg request me for review?

Probably because I added you to the package maintainer list. Thanks for your reviews!

Anyway, this look good to me as well, so I think we can get this merged at this point, if everyone is ok with it. @wucke13 is there anything else you think I should change ?

Copy link
Contributor

wucke13 left a comment

Looks good to me.
Not necessary but something about the cosmetics again:
I prefer to have my deps sorted by nativeBuildInputs (stdenv, cmake, ...), then buildInputs (libxyz, ...) and then check and postBuild deps. All of these groups sorted by alphanumerical order (but stripping away very generic prefixes like lib. That is just a personal preference, but I think it looks nice plus deps are named in the same order in the arguments and the vars, so that checking for deps which are mentioned in the arguments but not mentioned anywhere in the build are easier to spot by eye.

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Oct 29, 2019

@GrahamcOfBorg build boxfort criterion

@matthewbauer matthewbauer merged commit 9488fe0 into NixOS:master Nov 7, 2019
16 checks passed
16 checks passed
boxfort, criterion on aarch64-linux No attempt
Details
boxfort, criterion on x86_64-darwin No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
boxfort, criterion 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
@Thesola10

This comment has been minimized.

Copy link
Contributor

Thesola10 commented Nov 8, 2019

can't wait for it to reach nixpkgs, I'm using my "homemade" Criterion derivation in a Nix shell, having a more official package will simplify things 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.