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

pyspread: Correct dependencies and make wrapper #94537

Open
wants to merge 5 commits into
base: master
from

Conversation

@ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Aug 2, 2020

Change dependent python version to 3.6-3.9
Change dependent widget toolkit to pyqt5
Add a wrapper so as to execute the application from $out/bin.
Maintainers needed.

Motivation for this change

pyspread 1.99.x depends on Python 3.6-3.9 and pyqt5 instead of Python 2.7, wxPython or pygtk. Some dependent packages have also dropped Python2 support.
A wrapper is made using overrideAttrs and pythonFull.withPackages to make it executable out of the box.

It is the first time for me to package a Python application (rewrite the expressions of a package) in nixpkgs.
There might be some better or more efficient ways to do so.

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/) (Tested python36Packages.pyspread)
  • Determined the impact on package closure size (by running nix path-info -S before and after) (The closure size of pyspread is now 706115728)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@ofborg ofborg bot added the 6.topic: python label Aug 2, 2020
@ShamrockLee
Copy link
Contributor Author

@ShamrockLee ShamrockLee commented Aug 2, 2020

@ShamrockLee ShamrockLee force-pushed the ShamrockLee:pyspread branch from 24bbcf8 to ec4ccf6 Aug 14, 2020
@ShamrockLee ShamrockLee changed the title pyspread: Correct dependencies and make wrapper [WIP] pyspread: Correct dependencies and make wrapper Aug 14, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Aug 14, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/need-help-wrapping-pyspread/8573/1

ShamrockLee added 2 commits Aug 2, 2020
Change dependent python version to 3.6-3.9
Change dependent widget toolkit to pyqt5

Todo: Add a wrapper so as to execute the application from `$out/bin`.
pyspread-unwrapped is supposed to be imported as a module,
while pyspread is supposed to be executed as an application.

Rename default.nix to unwrapped.nix
and remove unnecesary dependencies
This is where `pyspread-unwrapped` is called.
Create default.nix for the executable application,
symlink share items and put desktop file into it,
and try to wrap with mkDerivationWith and wrapQtApp.
This is where `pyspread` is called.
@ShamrockLee ShamrockLee force-pushed the ShamrockLee:pyspread branch from ec4ccf6 to bee5ea0 Aug 15, 2020
@ShamrockLee ShamrockLee changed the title [WIP] pyspread: Correct dependencies and make wrapper pyspread: Correct dependencies and make wrapper Aug 15, 2020
@ShamrockLee ShamrockLee marked this pull request as ready for review Aug 15, 2020
@ShamrockLee ShamrockLee requested review from FRidh and jonringer as code owners Aug 15, 2020
Copy link
Contributor

@jonringer jonringer left a comment

If this is just meant to be just the application, and you don't need to export the package as a python package, then it should probably be moved out of python-modules and packaged as an application. Please see https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/python.section.md#building-packages-and-applications and https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/python.section.md#buildpythonapplication-function

then you would just do something like:

mkDerivationWith buildPythonApplication rec {
 ... 
}
pkgs/development/python-modules/pyspread/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pyspread/default.nix Outdated Show resolved Hide resolved
@ShamrockLee
Copy link
Contributor Author

@ShamrockLee ShamrockLee commented Aug 16, 2020

@jonringer Thank you for reviewing.
PySpread runs Python expressions users have written inside the cells, and people may want to write those expressions with different versions of Python interpreter.

Users may also want extra packages when writing those expressions. Since pip install (the usual way for PySpread to install packages for users) won't work in NixOS, I set an argument for users to specify their custom package set.

There is also an API to be imported in python scripts, and that's the reason I preserve pyspread-unwrapped.

@ShamrockLee
Copy link
Contributor Author

@ShamrockLee ShamrockLee commented Aug 16, 2020

I have changed the way it is wrapped and adopted the above suggestions.
@ofborg eval
@ofborg build python36Packages.pyspread python37Packages.pyspread python38Packages.pyspread python39Packages.pyspread pyspread-app

@ShamrockLee ShamrockLee requested a review from jonringer Aug 16, 2020
The python package is now called `pyspread` to meet the convensions
and pyspread-app is the new name of the executable package.
Rewrite the wrapping part to adopt the advice from the reviewer
and to make it more clearly and easier to compile.
Package the wrapping process as a function
available for NixOS or home-manareg modules and users.
@ShamrockLee ShamrockLee force-pushed the ShamrockLee:pyspread branch from c4b4017 to d3873f4 Aug 16, 2020
pkgs/development/python-modules/pyspread/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pyspread/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/pyspread-app/wrap-pyspread.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/pyspread-app/wrap-pyspread.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/pyspread-app/wrap-pyspread.nix Outdated Show resolved Hide resolved
@@ -22235,6 +22235,9 @@ in

pybitmessage = callPackage ../applications/networking/instant-messengers/pybitmessage { };

wrapPyspread = libsForQt5.callPackage ../applications/misc/pyspread-app/wrap-pyspread.nix { };

This comment has been minimized.

@FRidh

FRidh Aug 18, 2020
Member

Expose only a single attribute, the application that works.

This comment has been minimized.

@ShamrockLee

ShamrockLee Aug 20, 2020
Author Contributor

When writing a NixOS module or a home-manager, should I call the file wrap-pyspread.nix directly?

This comment has been minimized.

@jonringer

jonringer Aug 21, 2020
Contributor

no, you can reference pkgs.pyspread

This comment has been minimized.

@ShamrockLee

ShamrockLee Aug 22, 2020
Author Contributor

@jonringer I tried for a while, but couldn't find a way to override pkgs.pyspread-app and change its pythonWithDependencies and customInterpreterName attributes.
How should I modify the wrapping expressions and write the override one to achieve this?

This comment has been minimized.

@ShamrockLee

ShamrockLee Aug 24, 2020
Author Contributor

I modified the program to make pyspread-app a set, and move the executable application to pyspread-app.app.

@ShamrockLee ShamrockLee requested a review from FRidh Aug 21, 2020
@zowoq
Copy link
Contributor

@zowoq zowoq commented Aug 22, 2020

@ofborg eval

Change the output way to make overriding easier.
pyspread-app is now a set containing
pyspread-app.app: the workable application
pyspread-app.wrapPyspread: the wrapping function
.pickRequiredDeps, .pickOptionalDeps:
functions to get deps from the given package set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.