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

Jump to bottom

pythonPackages.qscintilla: enable for python3 #54834

Merged
merged 1 commit into from Jan 30, 2019
Merged

pythonPackages.qscintilla: enable for python3 #54834

merged 1 commit into from Jan 30, 2019

Conversation

lsix
Copy link
Member

@lsix lsix commented Jan 28, 2019

Motivation for this change

I plan to add Qgis3 into nixpkgs, and for that qscintilla for python3 is required.

This work is somehow based on #38022

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.

@lsix lsix requested a review from FRidh as a code owner January 28, 2019 19:54
dotlambda
dotlambda reviewed Jan 29, 2019
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

I'd prefer using a unified expression for both, Python 2 and 3. Can't we use pyqt5 for both?

pkgs/development/python-modules/qscintilla/py3.nix Outdated
meta = with pkgs.lib; {
description = "A Python binding to QScintilla, Qt based text editing control";
license = licenses.lgpl21Plus;
maintainers = with maintainers; [ danbst ];
Copy link
Member

Choose a reason for hiding this comment

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

That's not you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed.

I propagated the value of the current maintainer of the python2 only version. I’ll add myself shortly.

@lsix
Copy link
Member Author

lsix commented Jan 29, 2019

@dotlambda I used this approach (python2+pyqt4 / python3+pyqt5) to minimize the impact on existing derivations. I did not check yet what would be the impact of migrating the python2 binding to qt5 inside nixpkgs, but it will be a breaking change for people relying on it outside of nixpkgs.

I’ll have a look ASAP at it and let you know what are the impacts within nixpkgs.

@dotlambda
Copy link
Member

If it's not possible to use pyqt5 everywhere, I suggest having 4 packages: python2.pkgs.qscintilla-qt4, python2.pkgs.qscintilla-qt5, python3.pkgs.qscintilla-qt4 and python3.pkgs.qscintilla-qt5.

@danbst
Copy link
Contributor

danbst commented Jan 29, 2019

I've tried to upgrade pyqt4 to pyqt5 (tortoisehg, uses pyqscintilla) and failed. So I agree, let's leave qscintilla as qt4 one, and rather add qscintilla-qt5 as a new package. It builds with python2 BTW.

@lsix
Copy link
Member Author

lsix commented Jan 29, 2019

@dotlambda I have pushed an updated version, with both qscintilla-qt4 and qscintilla-qt5.

dotlambda
dotlambda requested changes Jan 29, 2019
View reviewed changes
pkgs/development/python-modules/qscintilla-qt5/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/qscintilla-qt5/default.nix Show resolved Hide resolved
pkgs/development/python-modules/qscintilla-qt5/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/qscintilla-qt5/default.nix Outdated
description = "A Python binding to QScintilla, Qt based text editing control";
license = licenses.lgpl21Plus;
maintainers = with maintainers; [ lsix ];
platforms = platforms.unix;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for specifying this? buildPythonPackage will set an appropriate default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I re-used what was already present in pkgs/development/python-modules/qscintilla/default.nix. I can leave it unspecified.

@lsix lsix force-pushed the enable-qscintilla-py3 branch 3 times, most recently from d49088e to eadab46 Compare January 29, 2019 22:15
@lsix
Copy link
Member Author

lsix commented Jan 29, 2019

updates done.

dotlambda
dotlambda reviewed Jan 30, 2019
View reviewed changes
pkgs/development/python-modules/qscintilla-qt5/default.nix Outdated
src = qscintillaCpp.src;
format = "other";

buildInputs = [ lndir qscintillaCpp sip ];
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't sip be propagated?

Copy link
Member Author

Choose a reason for hiding this comment

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

sip is actually a binding generator tool. Is is not required at runtime. It generates artefacts. I am just not sure if it should go into nativeBuildInputs of if buildInputs is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opted for the nativeBuildInputs approach

Copy link
Member

Choose a reason for hiding this comment

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

I'm still not sure about that since I don't know anything about cross-compiling Python packages.

dotlambda
dotlambda approved these changes Jan 30, 2019
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

LGTM. I didn't test it in any way though.

@danbst danbst merged commit 4f8e9e7 into NixOS:master Jan 30, 2019
@das-g
Copy link
Member

das-g commented Feb 21, 2019

I plan to add Qgis3 into nixpkgs

Cool! I'm looking forward to having it available on NixOS. #35669

Can we help in any way?

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

@dotlambda dotlambda dotlambda approved these changes

@danbst danbst Awaiting requested review from danbst

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lsix @dotlambda @danbst @das-g @FRidh @GrahamcOfBorg