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

Closed
wants to merge 3 commits into from
Closed

Conversation

@BrunoChevalier
Copy link

BrunoChevalier commented Aug 11, 2018

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.

pkgs/development/libraries/boxfort/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/boxfort/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/boxfort/default.nix Outdated

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

This comment has been minimized.

Copy link
@symphorien

symphorien Aug 11, 2018

Contributor

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

pkgs/development/libraries/criterion/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/csptr/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/csptr/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/csptr/default.nix Outdated Show resolved Hide resolved
@BrunoChevalier
Copy link
Author

BrunoChevalier commented Aug 11, 2018

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
Contributor

symphorien commented Aug 11, 2018

it is enough to force-push your rephrased commits.

@symphorien
Copy link
Contributor

symphorien commented Aug 11, 2018

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

@BrunoChevalier
Copy link
Author

BrunoChevalier commented Aug 11, 2018

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

Experimental library, not versioned, defaulted to 0.0.1
@BrunoChevalier BrunoChevalier force-pushed the BrunoChevalier:criterion_package branch to b2628f2 Aug 11, 2018
@BrunoChevalier
Copy link
Author

BrunoChevalier commented Aug 11, 2018

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
Contributor

symphorien commented Aug 14, 2018

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.

Copy link
Member

ryantm left a comment

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.

{stdenv, pkgconfig, fetchFromGitHub, cmake, dyncall, nanomsg, boxfort, csptr, libgit2} :

stdenv.mkDerivation {
name = "criterion-2.3.2";

This comment has been minimized.

Copy link
@ryantm

ryantm Feb 24, 2019

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

This comment has been minimized.

Copy link
@ryantm

ryantm Feb 24, 2019

Member

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.

src = fetchFromGitHub {
owner = "Snaipe";
repo = "Criterion";
rev = "9b70365825aced7333d7867bb5c64c63919ce510";

This comment has been minimized.

Copy link
@ryantm

ryantm Feb 24, 2019

Member
Suggested change
rev = "9b70365825aced7333d7867bb5c64c63919ce510";
rev = "v${version}";
{stdenv, fetchFromGitHub, cmake} :

stdenv.mkDerivation {
name = "csptr-2.0.4";

This comment has been minimized.

Copy link
@ryantm

ryantm Feb 24, 2019

Member
Suggested change
name = "csptr-2.0.4";
pname = "csptr";
version = "2.0.4";
src = fetchFromGitHub {
owner = "Snaipe";
repo = "libcsptr";
rev = "82d3b1beafe7ed9f13f75e0d3367c60205e10f27";

This comment has been minimized.

Copy link
@ryantm

ryantm Feb 24, 2019

Member
Suggested change
rev = "82d3b1beafe7ed9f13f75e0d3367c60205e10f27";
rev = "v${version}";
{stdenv, fetchFromGitHub, cmake} :

stdenv.mkDerivation {
name = "boxfort-0.0.1";

This comment has been minimized.

Copy link
@ryantm

ryantm Feb 24, 2019

Member
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
8 of 10 tasks complete
@Thesola10
Copy link
Contributor

Thesola10 commented Oct 7, 2019

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 label Jun 1, 2020
@Thesola10 Thesola10 closed this Jun 1, 2020
@BrunoChevalier BrunoChevalier deleted the BrunoChevalier:criterion_package branch Jun 2, 2020
@BrunoChevalier
Copy link
Author

BrunoChevalier commented Jun 2, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.