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.

Sign up for GitHub

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

Jump to bottom

pythonPackages.pyqt5: fix sip dependency #52613

Merged
merged 2 commits into from Dec 21, 2018
Merged

pythonPackages.pyqt5: fix sip dependency #52613

merged 2 commits into from Dec 21, 2018

Conversation

nyanloutre
Copy link
Member

@nyanloutre nyanloutre commented Dec 21, 2018 •
edited

Motivation for this change

Related to the discution in #49400

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

FRidh
FRidh previously requested changes Dec 21, 2018
View reviewed changes
pkgs/development/python-modules/pyqt/5.x.nix Outdated
@@ -65,6 +65,8 @@ in buildPythonPackage {
'';

postInstall = ''
ln -s ${sip}/${python.sitePackages}/sip.pyi $out/${python.sitePackages}/PyQt5
ln -s ${sip}/${python.sitePackages}/sip.so $out/${python.sitePackages}/PyQt5
Copy link
Member

Choose a reason for hiding this comment

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

replace .so with ${stdenv.hostPlatform.extensions.sharedLibrary}

Copy link
Member Author

Choose a reason for hiding this comment

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

done 👍

@FRidh FRidh mentioned this pull request Dec 21, 2018
Closed
@nyanloutre nyanloutre force-pushed the pyqt5-fix branch 2 times, most recently from 093a322 to e5f164e Compare December 21, 2018 10:42
@timokau
Copy link
Member

timokau commented Dec 21, 2018

@GrahamcOfBorg eval

1 similar comment
@timokau
Copy link
Member

timokau commented Dec 21, 2018

@GrahamcOfBorg eval

@timokau
Copy link
Member

timokau commented Dec 21, 2018

@GrahamcOfBorg build python2.pkgs.pyqt5 python3.pkgs.pyqt5

@nyanloutre
Copy link
Member Author

nyanloutre commented Dec 21, 2018 •
edited

Sorry I had to rewrite it a bit. I first tried to skip the module rename by only linking to the library but this wasn't enough.

I didn't try to find side effects of this name changing though

@timokau
Copy link
Member

timokau commented Dec 21, 2018

@GrahamcOfBorg build python2.pkgs.pyqt5 python3.pkgs.pyqt5

@timokau
Copy link
Member

timokau commented Dec 21, 2018

@GrahamcOfBorg build calibre

@timokau
Copy link
Member

timokau commented Dec 21, 2018

The qtwebkit failures on darwin and aarch64 are unrelated and handled in #51846.

Thanks for the fix!

@timokau timokau merged commit bfca708 into NixOS:master Dec 21, 2018
@nyanloutre nyanloutre deleted the pyqt5-fix branch December 21, 2018 14:30
@veprbl
Copy link
Member

veprbl commented Dec 21, 2018

I wonder if --sip-module needs to be applied conditionally to preserve compatibility for those who use sip without pyqt5. Also, perhaps, we should now move sip out of pyqt5.propagatedBuildInputs to just pyqt5.buildInputs so that sip can't clash with pyqt5. Also sip and pyqt5 currently often go together in dependencies of other packages, we would probably need to eliminate those references to sip as well.

@orivej
Copy link
Contributor

orivej commented Dec 25, 2018

This broke tortoisehg which uses pyqt4 rather than pyqt5.

@orivej
Copy link
Contributor

orivej commented Dec 25, 2018

It seems that pyqt5 depends on https://pypi.org/project/PyQt5-sip/ rather than on https://pypi.org/project/SIP/.

@orivej orivej mentioned this pull request Dec 26, 2018
Closed
10 tasks
@orivej
Copy link
Contributor

orivej commented Dec 26, 2018

Possible fix: #52897

@veprbl veprbl mentioned this pull request Dec 27, 2018
Closed
10 tasks
orivej added a commit to orivej/nixpkgs that referenced this pull request Dec 27, 2018
@orivej
pythonPackages.sip: rename the module from PyQt5.sip back to sip
ce3080c 
PyQt5 5.11 wants a copy of sip in PyQt5.sip to ensure that sip-generated PyQt5
modules have the correct sip runtime version. This issue is not relevant to
Nixpkgs, therefore PyQt5 may be reverted to use the common sip runtime.

This is an alternative to NixOS#52613 that does not break packages that need to import sip.

Fixes the build of tortoisehg.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nyanloutre @timokau @veprbl @orivej @FRidh @GrahamcOfBorg