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

cadical: init at 1.2.1 #83224

Merged
merged 2 commits into from Apr 4, 2020
Merged

cadical: init at 1.2.1 #83224

merged 2 commits into from Apr 4, 2020

Conversation

shnarazk
Copy link
Contributor

Motivation for this change

CaDiCaL is one of the best and art-of-the-state SAT solvers. It got a very good result in the SAT Race 2019, comparing with /pkgs/applications/science/logic/{minisat, cryptominisat, glucose}. So I think it's worth adding to nix packages.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

puzzlewolf
puzzlewolf approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@puzzlewolf puzzlewolf left a comment

Choose a reason for hiding this comment

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

Reviewed points
  • package name fits guidelines
  • package version fits guidelines
  • package build on x86_64-linux
  • executables tested on x86_64-linux
  • all depending packages build (ran nix-shell -p nixpkgs-review --run "nixpkgs-review pr 83224")
Possible improvements

Maybe remove those commented-out lines.

pkgs/applications/science/logic/cadical/default.nix Outdated Show resolved Hide resolved
puzzlewolf
puzzlewolf suggested changes Mar 28, 2020
View reviewed changes
Copy link
Contributor

@puzzlewolf puzzlewolf left a comment

Choose a reason for hiding this comment

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

This should only be two commits:
cadical: init at 1.2.1
maintainers: add shnarazk

Please rearrange accordingly :)

@shnarazk
Copy link
Contributor Author

@puzzlewolf thank you for checking! I've rebuilt the branch (after resetting...).

puzzlewolf
puzzlewolf approved these changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@puzzlewolf puzzlewolf left a comment

Choose a reason for hiding this comment

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

Apart from the last nitpick, LGTM :)

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/133

@timokau timokau self-requested a review April 4, 2020 11:25
timokau
timokau requested changes Apr 4, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just some nitpicks. The only important thing is fixing the rev.

pkgs/applications/science/logic/cadical/default.nix Outdated
src = fetchFromGitHub {
owner = "arminbiere";
repo = pname;
rev = "92d72896c49b30ad2d50c8e1061ca0681cd23e60";
Copy link
Member

Choose a reason for hiding this comment

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

This should point to a release tag instead of a commit hash. Otherwise automatic updates won't work and its easy to get a mismatch between version and rev.

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 = "92d72896c49b30ad2d50c8e1061ca0681cd23e60";
rev = "rel-${version}";

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the review and explanation :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing! I revised it.

pkgs/applications/science/logic/cadical/default.nix Outdated

src = fetchFromGitHub {
owner = "arminbiere";
repo = pname;
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
repo = pname;
repo = "cadical";

repo and pname are the same, but they don't necessarily change together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly. I did.

pkgs/applications/science/logic/cadical/default.nix Outdated
maintainers = with maintainers; [ shnarazk ];
platforms = platforms.unix;
license = licenses.mit;
homepage = http://fmv.jku.at/cadical;
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
homepage = http://fmv.jku.at/cadical;
homepage = "http://fmv.jku.at/cadical";

NixOS/rfcs#45

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did at c5a12e3 .

timokau
timokau approved these changes Apr 4, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Thanks @shnarazk, @puzzlewolf!

@timokau timokau merged commit 02b1f2a into NixOS:master Apr 4, 2020
@shnarazk shnarazk requested a review from timokau April 4, 2020 15:50
@shnarazk
Copy link
Contributor Author

shnarazk commented Apr 4, 2020

Thank you too @timokau and @puzzlewolf !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@puzzlewolf puzzlewolf puzzlewolf approved these changes

@timokau timokau Awaiting requested review from timokau

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 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
5 participants
@shnarazk @nixos-discourse @timokau @puzzlewolf @Janik-Haag