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.

Sign up for GitHub

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 package #44911

Closed
wants to merge 3 commits into from
Closed

Criterion package #44911

wants to merge 3 commits into from
+73 −0

Conversation

BrunoChevalier
Copy link

@BrunoChevalier BrunoChevalier commented Aug 11, 2018 •
edited
Loading

Motivation for this change

I wanted to use Criterion as my unit testing framework, but it wasn't present yet as a nixos package.
These are the first packages I've added, so it's best if someone thoroughly looks at this request.

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

symphorien
symphorien reviewed Aug 11, 2018
View reviewed changes
pkgs/development/libraries/boxfort/default.nix Outdated

meta = {
homepage = "https://github.com/diacritic/BoxFort";
description = "A simple, cross-platform sandboxing C library powering Criterion.";
Copy link
Member

Choose a reason for hiding this comment

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

The description should not end with a period: see https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md

@BrunoChevalier
Copy link
Author

BrunoChevalier commented Aug 11, 2018 •
edited
Loading

I guess I'll also need to reword the commit messages to comply with the CONTRIBUTING.md file.
I'll close this request and will issue a new one with modified changesets.

@symphorien Thanks for the review!

@symphorien
Copy link
Member

it is enough to force-push your rephrased commits.

@symphorien
Copy link
Member

symphorien commented Aug 11, 2018 •
edited
Loading

Also, have you tried setting doCheck=true on any of these packages ?

@BrunoChevalier
Copy link
Author

No, not yet. I'll go over the complete checklist to see what I can do.

@BrunoChevalier
boxfort: init at 0.0.1

Verified

This commit was signed with the committer’s verified signature.
renovate-bot Mend Renovate
GPG key ID: 81C63AE1C224182B
Verified
Learn about vigilant mode
2fa8d03 
Experimental library, not versioned, defaulted to 0.0.1
@BrunoChevalier
csptr: init at 2.0.4
fe6912e
@BrunoChevalier
criterion: init at 2.3.2
b2628f2
@BrunoChevalier
Copy link
Author

Ok, doCheck=true fails for boxfort. I'm trying to figure out why the tests fail.
The other packages don't have a "make check" target.

@symphorien
Copy link
Member

symphorien commented Aug 14, 2018 •
edited
Loading

I think the policy for unversioned packages is to use the date of the last commit as a version: https://nixos.org/nixpkgs/manual/#sec-package-naming
Apart from this, LGTM.

ryantm
ryantm requested changes Feb 24, 2019
View reviewed changes
Copy link
Member

@ryantm ryantm left a comment

Choose a reason for hiding this comment

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

I'm requesting these changes so that versions can be updated more easily. Boxfort needs to be unstable because as far as I can tell 0.0.1 is not a valid upstream version.

Please also consider adding yourself as a maintainer of these packages.

pkgs/development/libraries/criterion/default.nix
{stdenv, pkgconfig, fetchFromGitHub, cmake, dyncall, nanomsg, boxfort, csptr, libgit2} :

stdenv.mkDerivation {
name = "criterion-2.3.2";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name = "criterion-2.3.2";
pname = "criterion";
version = "2.3.2";

Copy link
Member

Choose a reason for hiding this comment

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

FYI, 2.3.3 is out now. It's not a requirement that you have it at the latest version to get this merged though.

pkgs/development/libraries/criterion/default.nix
src = fetchFromGitHub {
owner = "Snaipe";
repo = "Criterion";
rev = "9b70365825aced7333d7867bb5c64c63919ce510";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rev = "9b70365825aced7333d7867bb5c64c63919ce510";
rev = "v${version}";

pkgs/development/libraries/csptr/default.nix
{stdenv, fetchFromGitHub, cmake} :

stdenv.mkDerivation {
name = "csptr-2.0.4";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name = "csptr-2.0.4";
pname = "csptr";
version = "2.0.4";

pkgs/development/libraries/csptr/default.nix
src = fetchFromGitHub {
owner = "Snaipe";
repo = "libcsptr";
rev = "82d3b1beafe7ed9f13f75e0d3367c60205e10f27";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rev = "82d3b1beafe7ed9f13f75e0d3367c60205e10f27";
rev = "v${version}";

pkgs/development/libraries/boxfort/default.nix
{stdenv, fetchFromGitHub, cmake} :

stdenv.mkDerivation {
name = "boxfort-0.0.1";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name = "boxfort-0.0.1";
pname = "boxfort";
version = "unstable-2018-05-10";

@mmahut
Copy link
Member

mmahut commented Aug 25, 2019

Are there any updates on this pull request, please?

@Thesola10 Thesola10 mentioned this pull request Oct 7, 2019
Closed
10 tasks
@Thesola10
Copy link
Contributor

I am working on a new derivation (#70609) and wanted to tell you, your csptr derivation looks unnecessary, since libcsptr already exists

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@Thesola10 Thesola10 closed this Jun 1, 2020
@BrunoChevalier BrunoChevalier deleted the criterion_package branch June 2, 2020 22:18
@BrunoChevalier
Copy link
Author

I forgot about this pull request.
This is obsolete at this point in time as @Thesola10 and @Yumasi have added and are maintaining working derivations for Criterion and Boxfort.
I stepped away of using NixOS as my OS in the meantime because it's too rigid for me to get things done quickly.
I love the ideas but I needed more flexibility and practicality.
In a couple of years I'll probably revisit NixOS.

@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@symphorien symphorien

@ryantm ryantm

Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@BrunoChevalier @symphorien @mmahut @Thesola10 @ryantm @GrahamcOfBorg @Janik-Haag