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.sip: rename the module from PyQt5.sip back to sip #52897

Closed
wants to merge 2 commits into from
Closed

pythonPackages.sip: rename the module from PyQt5.sip back to sip #52897

wants to merge 2 commits into from

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Dec 26, 2018

Motivation for this change

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 #52613 that does not break packages that need to import sip.

Fixes the build of tortoisehg.

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.

@orivej orivej requested a review from FRidh as a code owner December 26, 2018 03:38
@orivej orivej mentioned this pull request Dec 26, 2018
Merged
10 tasks
@veprbl
Copy link
Member

veprbl commented Dec 26, 2018

I don't think this is a good way to solve the issue. This introduces a patch that needs maintenance and may cause breakage for packages that will import PyQt5.sip.

The first commit is good IMO, I arrived at the same: 5a8b98f

@orivej
Copy link
Contributor Author

orivej commented Dec 26, 2018

Yes, it has to be maintained, but I think it is a right fix: we don't need a copy of sip in PyQt5.sip. (PyQt5 should have provided an option to build with global sip.) A package would import PyQt5.sip only if generated by sip -n PyQt5.sip, and packages take the -n PyQt5.sip part from PyQt5.QtCore.PYQT_CONFIGURATION, which will not contain -n PyQt5.sip after this commit. See Ultimaker/libArcus#76 and the linked mail for some details.

@orivej
Copy link
Contributor Author

orivej commented Dec 26, 2018

Since -n PyQt5.sip does not work with PyQt5 < 5.11, the projects will not adopt it unconditionally, and so far two projects have even rejected to support it:

and one has not responded to the bug report:

It seems that sticking to sip is currently the best.

@veprbl
Copy link
Member

veprbl commented Dec 26, 2018

@orivej You've convinced me that your approach is technically sound. But I still don't understand why we should insist on not providing PyQt5.sip module. We can provide both sip and PyQt5.sip without needing to do any patches, it seems.

@orivej
Copy link
Contributor Author

orivej commented Dec 26, 2018

It may be possible, but it is more complex to evaluate the result. I have tried to put this:

from __future__ import absolute_import
from sip import *

into PyQt5/sip.py, but pyrcc5 would not run because PyQt5.QtCore was missing some names… Other options to try are:

  • import sys, sip; sys.modules['PyQt5.sip'] = sip
  • symlink sip.so into PyQt5/sip.so: the operating system will load one shared library, but Python will think there are two
  • copy sip.so into PyQt5/sip.so: this does not make sense due to duplicate symbols
  • actually build PyQt5/sip.so with sip configured with --sip-module PyQt5.sip

The last approach is the most promising, but it is wasteful, and it is not clear whether it would work with applications that were not updated for -n PyQt5.sip and were built with the global sip: even though versions of the two sip will be exactly the same, they may need some global or thread local state for proper operation, and since this state will not be shared between the two libraries, they may not cooperate.

@veprbl veprbl mentioned this pull request Dec 27, 2018
Closed
10 tasks
@orivej
pythonPackages.pyqt5: delete obsolete substitutions
632028c 
The first is not present in configure, and the second has happened upstream.
@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.
@orivej orivej mentioned this pull request Dec 27, 2018
Merged
10 tasks
@orivej orivej closed this Dec 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh Awaiting requested review from FRidh

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
3 participants
@orivej @veprbl @GrahamcOfBorg