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

clingcon: init at 3.3.0 #80719

Merged
merged 1 commit into from Mar 9, 2020
Merged

Conversation

mucaho
Copy link
Contributor

@mucaho mucaho commented Feb 21, 2020

Motivation for this change

Clingcon is an answer set solver for constraint logic programs, building upon the answer set solver clingo. It extends the high-level modeling language of ASP with constraint solving capacities.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)

    • Tested using nix-build . --option build-use-chroot true -A clingcon
  • 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)

    • Not a nixos-level package, no virtual environment test necessary
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"

    • No package depends on this newly introduced package, nor does this change introduce any modifications to existing packages
    • Running this command throws an error for me
$ nix-shell -p nixpkgs-review --run "nixpkgs-review wip" --show-trace
error: while evaluating the attribute 'buildInputs' of the derivation 'shell' at /nix/store/sj4in86dg5bij894jywp4zygr31w0dv1-nixos-18.03.133402.cb0e20d6db9/nixos/pkgs/stdenv/generic/make-derivation.nix:148:11:
while evaluating 'chooseDevOutputs' at /nix/store/sj4in86dg5bij894jywp4zygr31w0dv1-nixos-18.03.133402.cb0e20d6db9/nixos/lib/attrsets.nix:460:22, called from undefined position:
undefined variable 'nixpkgs-review' at (string):1:94
  • Running a similar command nix-shell -I nixpkgs=. -p nixpkgs-review --run "nixpkgs-review wip" returns No diff detected, stopping review

  • Tested execution of all binary files (usually in ./result/bin/)

    • Tested also using runtime-sandboxing (nix-shell -I nixpkgs=. --pure -p clingcon)
  • Determined the impact on package closure size (by running nix path-info -S before and after)

    • This throws an error for me
error: Please be informed that this pseudo-package is not the only part of
Nixpkgs that fails to evaluate. You should not evaluate entire Nixpkgs
without some special measures to handle failing packages, like those taken
by Hydra.
  • Ensured that relevant documentation is up to date

    • Which documentation is meant here?
  • Fits CONTRIBUTING.md.

    • Everything fulfilled, except the maintainers field. While I'm happy to contribute and update the package version here and there I can not commit myself to maintaining this package.

@mucaho
Copy link
Contributor Author

mucaho commented Feb 21, 2020

@7c6f434c I have added you as a reviewer, since it looks like you introduced a similar package from potassco (clingo), so this might interest you too :)

@doronbehar
Copy link
Contributor

Which documentation is meant here?

If someone makes a PR that changes something substantial in NixOS, he might need to make changes to documentation. In the very frequent case of a new package there's no need to update documentation.

  • No package depends on this newly introduced package, nor does this change introduce any modifications to existing packages
  • Running this command throws an error for me

Yea this [ ] is kind of confusing. In general, as a contributor, you don't really need to tick all of those boxes, just tick what ever you feel is necessary and reviewers will generally cooperate. For new pacakges (or updates) what's most important is:

[ ] Tested execution of all binary files

And if you use Linux / NixOS, please build the package in a sandbox, (as it is by default with nix-daemon).

];

meta = {
inherit version;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is needed. Did you read it is required in the documentation? Most derivations don't have this.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is following what I did for clingo because the non-meta version attribute gets handled in a pretty messy way and the auto-updater script is easier to write if the version is provided in a clean place.

Of course this makes less sense when rev in the src is not version-derived anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

What auto-updater?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I sometimes use update-walker when I remember to update Clingo

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, good luck with that :) I tend to count on @r-ryantm to do the dirty job for me when possible...

pkgs/applications/science/logic/potassco/clingcon.nix Outdated Show resolved Hide resolved
pkgs/applications/science/logic/potassco/clingcon.nix Outdated Show resolved Hide resolved
pkgs/applications/science/logic/potassco/clingcon.nix Outdated Show resolved Hide resolved
pkgs/applications/science/logic/potassco/clingcon.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Show resolved Hide resolved
@7c6f434c
Copy link
Member

Thanks for the detailed review @doronbehar — I strongly agree with a couple of comments which I marked with 👍 and for the rest there is a question of how easy it is to build different versions etc.

@doronbehar
Copy link
Contributor

for the rest there is a question of how easy it is to build different versions etc.

What do you mean?

@7c6f434c
Copy link
Member

7c6f434c commented Feb 25, 2020

Well, two things I strongly agree with — I just want them fixed; but whether making a Python feature flag and a version-based rev is simple enough to implement — well, both cases are possible (good questions to ask, but I am used to the answer being «not worth it in the current situation»)

@doronbehar
Copy link
Contributor

I'm sorry but I don't understand. What do you mean by:

  • "build different versions"
  • "Python feature flag"
  • "version-based rev"

??

@7c6f434c
Copy link
Member

Python feature flag

enablePython

version-based rev

using a git revision specified not as an opaque hexadecimal hash

@mucaho
Copy link
Contributor Author

mucaho commented Mar 8, 2020

@doronbehar @7c6f434c Thank you for your detailed code reviews!

I have incorporated most suggestions and retested the binary, however

  • left the git revision to be the commit hash because of Michael's auto-update concerns
  • left the inherited version in the meta descriptions because of Michael's suggestion for easier auto-updated
  • did not include the lua and python bindings as it seems they will be deprecated in future

Is this OK now, or should we add further modifications?

@7c6f434c
Copy link
Member

7c6f434c commented Mar 8, 2020

If upstream doesn't provide a nicely named version tag for releases. then there is no point in having meta.version, and I wonder what is the best way to tell @r-ryantm that this expression is not automatically updateable.

@mucaho
Copy link
Contributor Author

mucaho commented Mar 8, 2020

@7c6f434c

If upstream doesn't provide a nicely named version tag for releases.

But it does -> The v3.3.0 tag points exactly to the desired revision d36fd.

@7c6f434c
Copy link
Member

7c6f434c commented Mar 8, 2020

Then it is better to use this tag in rev

@mucaho
Copy link
Contributor Author

mucaho commented Mar 8, 2020

Alrighty, tag is used in rev now instead.

];

meta = {
inherit version;
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, good luck with that :) I tend to count on @r-ryantm to do the dirty job for me when possible...

@7c6f434c 7c6f434c merged commit bbab732 into NixOS:master Mar 9, 2020
@mucaho mucaho deleted the feature/clingcon_init branch April 13, 2020 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants