-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
marcel: init at 0.22.2 #267461
marcel: init at 0.22.2 #267461
Conversation
Fixes #263995 |
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 requested some changes about using the latest 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.
LGTM
Please, could you restructure your commits, so that there are only 2 commits: one called maintainers: add kud
, which only has the maintainer-list.nix
change, and another one called marcel: init at 0.20.0
, which has every other change.
If you need help with this, I can help.
@TomaSajt now it seems only contains 2 commits. Thank you for your instructions! |
format = "setuptools"; And usually the these changes are split (ie 2 PRs):
|
No, they are usually one PR, adding to maintainers list isn't usually approved unless you have some package to maintain. |
No need for a new Pull Request. A commit suffices
This is at the discretion of the committers. E. G. Sometimes the person wants to contribute to multiple packages at once, then it is better to do a PR for maintainers list update first. |
now using I'm not sure why it seems I need to add setuptools into propagatedBuildInputs, according to this comment, |
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.
Thanks for the contribution and patience - one more point of feedback from my side before I'd consider it mergeable. Also FYI 0.22.2 is out
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.
looks good -- minor issue with setuptools, it should not get propagated as it is just used to build the package and the substituteInPlace is not needed as python3 is already in the path.
validated maintainer info matches:
curl -s https://api.github.com/users/KUD-00|jq '{name,github:.login,githubId:.id}'
Description of changes
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)It's my first time to contribute. I apologize first for any mistakes in the codes. I have tested the binary in /result/bin(in
nix-shell - A marcel
environment), but not sure if it is executable when installing it on any system. I tried$ nix-env -f . -iA marcel
but it said it can't findlib
and many other parameters. I'm not sure is it good to use/run/current-system/sw/bin/bash