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

Jump to bottom

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 Yumasi commented Oct 4, 2019 •
edited

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
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
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.

@Yumasi Yumasi requested a review from wucke13 October 9, 2019 09:27
@Thesola10 Thesola10 mentioned this pull request Oct 9, 2019
Closed
10 tasks
@Thesola10
Copy link
Contributor

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
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
Copy link
Contributor

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
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
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
Copy link
Contributor

Thesola10 commented Oct 9, 2019 •
edited

@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
maintainers: add Yumasi
e2c8c1a 
Signed-off-by: Guillaume Pagnoux <gpagnoux@gmail.com>
@Yumasi
boxfort: init at unstable-2019-09-19
b76e290 
Signed-off-by: Guillaume Pagnoux <gpagnoux@gmail.com>
@Yumasi
criterion: init at 2.3.3
b76cfaf 
Signed-off-by: Guillaume Pagnoux <gpagnoux@gmail.com>
@ofborg ofborg bot requested a review from Thesola10 October 9, 2019 13:05
@Thesola10
Copy link
Contributor

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

Thesola10
Thesola10 approved these changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@Thesola10 Thesola10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@Yumasi
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 ?

wucke13
wucke13 approved these changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@wucke13 wucke13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@GrahamcOfBorg build boxfort criterion

@matthewbauer matthewbauer merged commit 9488fe0 into NixOS:master Nov 7, 2019
@Thesola10
Copy link
Contributor

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 🎉

@nloomans nloomans mentioned this pull request Mar 12, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thesola10 Thesola10 Thesola10 approved these changes

@wucke13 wucke13 wucke13 approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Yumasi @wucke13 @Thesola10 @aanderse @matthewbauer @Janik-Haag