-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
python311Packages.abjad: fix build #311252
Conversation
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.
Note: I seem to be unable to run
nixpkgs_review
; thenix-env -qaP
command (with some extra flags) required for the script seems to fail and I'm not sure why.
It might have been killed by the OOM killer, I fixed it by allocating a few gigs of swap space. nixpkgs-review does seem to take up quite a lot of memory.
LGTM :)
Hello all, not sure what the process is for this, but how do I go about pinging people for review? I realize now this has been open for more than a month and I'd like to get it merged if possible |
Normally you would ask on the NixOS discourse, on a thread such as PRs ready for review. That will usually bring your PR to attention. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4154 |
Hey I just realised your branch has conflicts with the master branch, meaning it can't be merged into it cleanly. Rebase against nixpkgs master and solve the conflicts from there. Force-push to your PR branch and you should be good to go. |
Done I think! |
{ | ||
lib, | ||
buildPythonPackage, | ||
fetchPypi, | ||
ply, | ||
roman, | ||
uqbar, | ||
pythonOlder, | ||
pytestCheckHook, | ||
lilypond, | ||
{ lib | ||
, buildPythonPackage | ||
, fetchPypi | ||
, ply | ||
, roman | ||
, uqbar | ||
, pythonOlder | ||
, pytestCheckHook | ||
, lilypond | ||
, typing-extensions |
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.
Can you please remove this reformatting, it's unnecessary
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.
Even if it's specified that way in the code conventions section of the contributing page?
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.
Oh I didn't notice that bit, if you want to keep it please put the change in another commit. Also please change the PR title to something like abjad: fix build
.
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.
All done!
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.
Could you please revert this change?
We have adopted nixfmt-rfc-style
formatting for python modules. #313628
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.
@natsukium reverted!
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.
Awesome thanks for all that. 👍
Since this package is a python library, please write the commit messages and PR title starting with |
Done! |
Build failed with ModuleNotFoundError, fixing.
--replace now deprecated, changing to --replace-fail.
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, thanks!
Description of changes
Two changes:
abjad
build was broken with a ModuleNotFoundError, so I added the relative module to dependencies.--replace
option was deprecated, so I replaced it with--replace-fail
for robustness.Note: I seem to be unable to run
nixpkgs_review
; thenix-env -qaP
command (with some extra flags) required for the script seems to fail and I'm not sure why.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/
)Add a 👍 reaction to pull requests you find important.