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

Refactor rabbitvcs #102378

Merged
merged 3 commits into from Dec 6, 2020
Merged

Refactor rabbitvcs #102378

merged 3 commits into from Dec 6, 2020

Conversation

freezeboy
Copy link
Contributor

@freezeboy freezeboy commented Nov 1, 2020

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 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.

pkgs/development/python-modules/pysvn/default.nix Outdated Show resolved Hide resolved

# 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
Copy link
Member

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?

Copy link
Contributor Author

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

pkgs/development/python-modules/pysvn/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pycxx/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pycxx/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pysvn/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pysvn/default.nix Outdated Show resolved Hide resolved
'' + (if !stdenv.isDarwin then "" else ''
sed -i -e 's|libpython2.7.dylib|lib/libpython2.7.dylib|' Makefile
'');

checkPhase = "make -C ../Tests";
checkInputs = [ pkgs.glibcLocales ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
checkInputs = [ pkgs.glibcLocales ];
checkInputs = [ glibcLocales ];

Copy link
Contributor Author

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 ?

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 102378 run on x86_64-darwin 1

5 packages failed to build:
  • python27Packages.pycxx
  • python27Packages.pysvn
  • python37Packages.pycxx
  • python37Packages.pysvn
  • python38Packages.pysvn
1 package built:
  • python38Packages.pycxx

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 102378 run on x86_64-linux 1

4 packages failed to build:
  • python27Packages.pycxx
  • python27Packages.pysvn
  • python37Packages.pycxx
  • python37Packages.pysvn
3 packages built:
  • python38Packages.pycxx
  • python38Packages.pysvn
  • rabbitvcs

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 2, 2020

Result of nixpkgs-review pr 102378 run on x86_64-linux 1

7 packages built:
  • python27Packages.pycxx
  • python27Packages.pysvn
  • python37Packages.pycxx
  • python37Packages.pysvn
  • python38Packages.pycxx
  • python38Packages.pysvn
  • rabbitvcs

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 2, 2020

Result of nixpkgs-review pr 102378 run on x86_64-darwin 1

3 packages failed to build:
  • python27Packages.pysvn
  • python37Packages.pysvn
  • python38Packages.pysvn
3 packages built:
  • python27Packages.pycxx
  • python37Packages.pycxx
  • python38Packages.pycxx
@nix { "action": "setPhase", "phase": "buildPhase" }
building
build flags: SHELL=/nix/store/8i9kyqfxhk2s9dzx32x0sgdk7kyx9wjc-bash-4.4-p23/bin/bash
Info: Make generate_svn_error_codes.hpp
/nix/store/65gmh01007csxmd6bbkvas8zkhlw0k7n-python3-3.8.6/bin/python3.8 -u generate_svn_error_codes/create_svn_error_codes_hpp.py /nix/store/hhs26sndnd1pgfdjx28laj6z3a0mmpr0-subversion-1.12.2-dev/include/subversion-1
Compile: generate_svn_error_codes/generate_svn_error_codes.cpp into generate_svn_error_codes.o
/nix/store/8i9kyqfxhk2s9dzx32x0sgdk7kyx9wjc-bash-4.4-p23/bin/bash: g++: command not found
make: *** [Makefile:203: generate_svn_error_codes.o] Error 127

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 4, 2020

Result of nixpkgs-review pr 102378 run on x86_64-darwin 1

3 packages failed to build:
  • python27Packages.pysvn
  • python37Packages.pysvn
  • python38Packages.pysvn
3 packages built:
  • python27Packages.pycxx
  • python37Packages.pycxx
  • python38Packages.pycxx
building
build flags: SHELL=/nix/store/8i9kyqfxhk2s9dzx32x0sgdk7kyx9wjc-bash-4.4-p23/bin/bash
Info: Make generate_svn_error_codes.hpp
/nix/store/65gmh01007csxmd6bbkvas8zkhlw0k7n-python3-3.8.6/bin/python3.8 -u generate_svn_error_codes/create_svn_error_codes_hpp.py /nix/store/hhs26sndnd1pgfdjx28laj6z3a0mmpr0-subversion-1.12.2-dev/include/subversion-1
Compile: generate_svn_error_codes/generate_svn_error_codes.cpp into generate_svn_error_codes.o
/nix/store/8i9kyqfxhk2s9dzx32x0sgdk7kyx9wjc-bash-4.4-p23/bin/bash: g++: command not found
make: *** [Makefile:203: generate_svn_error_codes.o] Error 127

@freezeboy
Copy link
Contributor Author

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

@nixos-discourse
Copy link

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/368

Copy link
Contributor

@jonringer jonringer left a 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?

@jonringer
Copy link
Contributor

cc @SuperSandro2000 for darwin review

@jonringer
Copy link
Contributor

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)

Copy link
Contributor

@Mathnerd314 Mathnerd314 left a 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.

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 102378 run on x86_64-linux 1

7 packages built:
  • python27Packages.pycxx
  • python27Packages.pysvn
  • python37Packages.pycxx
  • python37Packages.pysvn
  • python38Packages.pycxx
  • python38Packages.pysvn
  • rabbitvcs

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 26, 2020

Result of nixpkgs-review pr 102378 run on x86_64-darwin 1

3 packages failed to build:
  • python27Packages.pysvn
  • python37Packages.pysvn
  • python38Packages.pysvn
3 packages built:
  • python27Packages.pycxx
  • python37Packages.pycxx
  • python38Packages.pycxx
building
build flags: SHELL=/nix/store/k89nm2jva0qmvd970f84wq2iq1iwm9bs-bash-4.4-p23/bin/bash
Info: Make generate_svn_error_codes.hpp
/nix/store/0mabx9absir73mmijrsv0dsyvyya463r-python3-3.8.6/bin/python3.8 -u generate_svn_error_codes/create_svn_error_codes_hpp.py /nix/store/4vvvp48hqg8pgm5pval8xcl0zqgp9aid-subversion-1.12.2-dev/include/subversion-1
Compile: generate_svn_error_codes/generate_svn_error_codes.cpp into generate_svn_error_codes.o
/nix/store/k89nm2jva0qmvd970f84wq2iq1iwm9bs-bash-4.4-p23/bin/bash: g++: command not found
make: *** [Makefile:203: generate_svn_error_codes.o] Error 127

Copy link
Contributor

@doronbehar doronbehar left a 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 = ''
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

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 .

Copy link
Contributor Author

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

@freezeboy
Copy link
Contributor Author

cc @doronbehar

@ofborg ofborg bot requested a review from Mathnerd314 December 5, 2020 09:41
Copy link
Contributor

@jonringer jonringer left a 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

@jonringer
Copy link
Contributor

@freezeboy ping me if no one merges in a day or so

@doronbehar
Copy link
Contributor

nixpkgs-review fails for me with:

builder for '/nix/store/sbyd6kjkpdvzhmaqgg9h22xyi3cc5sxa-pycxx-7.1.4.tar.gz.drv' failed with exit code 1; last 10 log lines:
  100   355  100   355    0     0     25      0  0:00:14  0:00:13  0:00:01   449
  100   154  100   154    0     0     10      0  0:00:15  0:00:14  0:00:01    10
  100   359  100   359    0     0     22      0  0:00:16  0:00:15  0:00:01   600
  100   138  100   138    0     0      8      0  0:00:17  0:00:15  0:00:02     8
  100   355  100   355    0     0     19      0  0:00:18  0:00:17  0:00:01   437
  100   154  100   154    0     0      8      0  0:00:19  0:00:18  0:00:01     8
  100   359  100   359    0     0     18      0  0:00:19  0:00:19 --:--:--   534
  100   138  100   138    0     0      6      0  0:00:23  0:00:20  0:00:03     6
  curl: (47) Maximum (20) redirects followed
  error: cannot download pycxx-7.1.4.tar.gz from any mirror

Probably happens only on my machine.

@freezeboy
Copy link
Contributor Author

Strange, it works like a charm on my side :/

@jonringer
Copy link
Contributor

jonringer commented Dec 6, 2020

@doronbehar that's probably a transient issue with pypi's hosting. I was able to build it earlier

@jonringer jonringer merged commit 8f94e65 into NixOS:master Dec 6, 2020
@freezeboy freezeboy deleted the refactor-rabbitvcs branch December 6, 2020 19:30
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

6 participants