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
clingcon: init at 3.3.0 #80719
Conversation
@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 :) |
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.
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 |
]; | ||
|
||
meta = { | ||
inherit version; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What auto-updater?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
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. |
What do you mean? |
Well, two things I strongly agree with — I just want them fixed; but whether making a Python feature flag and a version-based |
I'm sorry but I don't understand. What do you mean by:
?? |
using a git revision specified not as an opaque hexadecimal hash |
5a3a50a
to
d1dcceb
Compare
@doronbehar @7c6f434c Thank you for your detailed code reviews! I have incorporated most suggestions and retested the binary, however
Is this OK now, or should we add further modifications? |
If upstream doesn't provide a nicely named version tag for releases. then there is no point in having |
But it does -> The v3.3.0 tag points exactly to the desired revision d36fd. |
Then it is better to use this tag in |
d1dcceb
to
202c2a8
Compare
Alrighty, tag is used in |
]; | ||
|
||
meta = { | ||
inherit version; |
There was a problem hiding this comment.
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...
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
innix.conf
on non-NixOS linux)nix-build . --option build-use-chroot true -A clingcon
Built on platform(s)
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"
Running a similar command
nix-shell -I nixpkgs=. -p nixpkgs-review --run "nixpkgs-review wip"
returnsNo diff detected, stopping review
Tested execution of all binary files (usually in
./result/bin/
)nix-shell -I nixpkgs=. --pure -p clingcon
)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.
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.