-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
Refactor rabbitvcs #102378
Refactor rabbitvcs #102378
Conversation
|
||
# It is not only shebangs, some tests also write scripts dynamically | ||
# so it is easier to simply search and replace | ||
sed -i "s|/bin/bash|${pkgs.bash}/bin/bash|" ../Tests/test-*.sh |
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.
We can't use patchShebangs here?
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.
no test 04 is writing its own script for the tests so we would still need this additionnal sed, so no real interest in using patchShebangs
'' + (if !stdenv.isDarwin then "" else '' | ||
sed -i -e 's|libpython2.7.dylib|lib/libpython2.7.dylib|' Makefile | ||
''); | ||
|
||
checkPhase = "make -C ../Tests"; | ||
checkInputs = [ pkgs.glibcLocales ]; |
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.
checkInputs = [ pkgs.glibcLocales ]; | |
checkInputs = [ glibcLocales ]; |
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.
why do you want to remove the checkPhase ?
Result of 5 packages failed to build:
1 package built:
|
Result of 4 packages failed to build:
3 packages built:
|
bad862f
to
6c56325
Compare
Result of 7 packages built:
|
Result of 3 packages failed to build:
3 packages built:
|
Result of 3 packages failed to build:
3 packages built:
|
Ok I can't run the build on a darwin machine, I'd be grateful if someone can try to fix the build on this platform |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
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.
do you mind squashing the darwin fixup commit into the pysvn commit?
cc @SuperSandro2000 for darwin review |
we can also do the darwin fix in another PR. Doesn't make sense to force people to support hardware they don't have (and Apple doesn't make it easy for me to emulate) |
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 to me. I'm not using rabbitvcs anymore so if you want to take over maintainership of the package feel free.
Result of 7 packages built:
|
Result of 3 packages failed to build:
3 packages built:
|
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.
Mostly looks good.
NIX_CFLAGS_COMPILE="-I${pkgs.aprutil.dev}/include/apr-1"; | ||
NIX_CFLAGS_COMPILE="-I${aprutil.dev}/include/apr-1"; | ||
|
||
prePatch = '' |
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.
It'd be nice to explain this if possible.
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.
Not needed anymore in fact, I remove this line
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.
Then please squash that last commit into the commit that touches this file.
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.
@freezeboy so do you intend to remove this prePatch ? Even if not, perhaps since it's tests related it'd be best to use postPatch
, per https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/patch-phase.md .
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 thought your note was about the cflags, indeed I can move this sed call to postPatch
2a45c79
to
c7f5f93
Compare
cc @doronbehar |
c7f5f93
to
3acbf22
Compare
3acbf22
to
301b1da
Compare
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
https://github.com/NixOS/nixpkgs/pull/102378
7 packages built:
python37Packages.pycxx python37Packages.pysvn python38Packages.pycxx python38Packages.pysvn python39Packages.pycxx python39Packages.pysvn rabbitvcs
I think future improvements can be done later. But the PR is still a benefit
@freezeboy ping me if no one merges in a day or so |
Also remove dependency to python2
301b1da
to
e54e408
Compare
nixpkgs-review fails for me with:
Probably happens only on my machine. |
Strange, it works like a charm on my side :/ |
@doronbehar that's probably a transient issue with pypi's hosting. I was able to build it earlier |
Motivation for this change
Update the tool and support python3
part of #101964
Needed to update pysvn and add pycxx in the process
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)