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

Python: split buildPythonPackage into two functions #18143

Merged
merged 6 commits into from
Sep 1, 2016

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Aug 31, 2016

Motivation for this change

Currently we have only buildPythonPackage for setuptools and wheels installations. However, we still have quite some packages that do not use setuptools or wheels. This PR provides another function, mkPythonDerivation, that

  • sets pythonPath so buildEnv will function
  • adds the python interpreter which was typically missing
  • wraps the Python programs

thus solving issues like described in #17679.

A rebuild is needed.

@mention-bot
Copy link

@FRidh, thanks for your PR! By analyzing the annotation information on this pull request, we identified @domenkozar, @adnelson and @abbradar to be potential reviewers

@FRidh FRidh force-pushed the buildpythonpackage branch from 031d0d0 to 23dae99 Compare August 31, 2016 09:02
@FRidh
Copy link
Member Author

FRidh commented Aug 31, 2016

Packages to convert (see #17679):

  • PyXML
  • PyXAPI
  • bsddb
  • crypt
  • curses_panel
  • dbus
  • fedora_cert
  • gdbm
  • gst-python
  • libvirt
  • notify
  • pyblock
  • pycairo
  • pygobject
  • pygobject3
  • pygtksourceview
  • pyqt4
  • pyqt5
  • pyside
  • pysvn
  • pywebkitgtk
  • qscintilla
  • recursivePthLoader
  • sip
  • sip_4_16
  • readline (*)
  • curses (*)
  • sqlite3 (*)

@FRidh
Copy link
Member Author

FRidh commented Aug 31, 2016

cc @garbas @lancelotsix @layus

@FRidh
Copy link
Member Author

FRidh commented Aug 31, 2016

Can we agree on the name of the functions? I need to know this before going any further. If we agree I'll fix the whole list and then we should be able to make it in before we branch tomorrow.

@layus
Copy link
Member

layus commented Aug 31, 2016

@FRidh I like the idea to provide a custom mkDerivation.
After reading the commits, I must say that using the name mkDerivation is extremely confusing. I propose mkPythonDerivation ? In the above commits, it is difficult to tell from the context if mkDerivation comes from python or from stdenv.

@FRidh FRidh force-pushed the buildpythonpackage branch from a4b5e3b to 5d85100 Compare August 31, 2016 09:41
@FRidh
Copy link
Member Author

FRidh commented Aug 31, 2016

@layus Thanks for the feedback and good point. I hope we eventually have `python.mkDerivation' like is now the case with Haskell and therefore chose that name. But changing it is no problem.

@layus
Copy link
Member

layus commented Aug 31, 2016

Yes, python.mkDerivation looks good at first, but if people do like below, then the ambiguity persists :-).

{ ... }:
with stdenv;
with python;

...

mkDerivation { 
    ...
}

@lsix
Copy link
Member

lsix commented Aug 31, 2016

@FRidh that's great (and will a good answer to a question I saw recently on the ml http://lists.science.uu.nl/pipermail/nix-dev/2016-August/021466.html).

I do not have a strong opinion about mkPythonDerivation vs mkDerivation. I am not sure how often someone will end-up with both stdenv and python “withed”. Seems to me that most cases will be like what you did when updating the first derivations:

  • import lib instead of all stdenv
  • use a pythonPackage set argument and build from its mkDerivation.

That said, I have no objection calling the function mkPythonDerivation to make sure there will never be ambiguities.

I will try have a closer look later on the whole PR later today.

@adnelson
Copy link
Contributor

Yeah I second the opposition of mkDerivation. I'm not a fan of it with the Haskell ecosystem either. mkDerivation to me is a sort of "god function" 😝 Honestly I'm not sure what's wrong with buildPythonPackage, although I think something like mkPythonDerivation is fine too.

@@ -536,6 +536,7 @@ All parameters from `mkDerivation` function are still supported.
* `installFlags`: A list of strings. Arguments to be passed to `pip install`. To pass options to `python setup.py install`, use `--install-option`. E.g., `installFlags=["--install-option='--cpp_implementation'"].
* `format`: Format of the source. Options are `setup` for when the source has a `setup.py` and `setuptools` is used to build a wheel, and `wheel` in case the source is already a binary wheel. The default value is `setup`.
* `catchConflicts` If `true`, abort package build if a package name appears more than once in dependency tree. Default is `true`.
* `checkInputs` Dependencies needed for running the `checkPhase`. These are added to `buildInputs` when `doCheck = true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this conceptually, but is there really anything wrong with just deeming these to be buildInputs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. As you can read, they are put together anyway. It might save some building when you disable tests. Furthermore, I'm still playing with the idea of separating the building and installing/testing in two separate derivations.

@FRidh
Copy link
Member Author

FRidh commented Aug 31, 2016

Thanks for the feedback.

A bit more about the naming. Looking at what the current mkDerivation and buildPythonPackage do, could you imagine better names for the both? These two names (or mkPythonDerivation) are quite different but they completely don't explain what their difference is.

@FRidh FRidh added this to the 16.09 milestone Sep 1, 2016
@FRidh FRidh self-assigned this Sep 1, 2016
@FRidh FRidh force-pushed the buildpythonpackage branch 3 times, most recently from f342972 to 77b8bed Compare September 1, 2016 11:42
@FRidh FRidh force-pushed the buildpythonpackage branch from 77b8bed to ae43d07 Compare September 1, 2016 12:34
1. mkDerivation which is used when the source is without setup.py and
not a wheel
2. buildPythonPackage which is used as before and calls mkDerivation
@FRidh FRidh force-pushed the buildpythonpackage branch from ae43d07 to 9a85190 Compare September 1, 2016 14:16
@FRidh FRidh merged commit 2a3077d into NixOS:master Sep 1, 2016
@FRidh FRidh deleted the buildpythonpackage branch September 19, 2016 11:24
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.

5 participants