Created attachment 330280 [details] [review]
Allow installing with pip

This commit adds a setup.py file which just calls the autotools to
configure/make/make install.

It is heavily inspired by the similar work from Simon McVittie on
dbus-python.

To try this out, I've pushed it to a temporary github repo https://github.com/bochecha/pygobject in the pip-install branch.

This allows installing pygobject with pip:

  pip install git+https://github.com/bochecha/pygobject.git@pip-install#egg=pygobject

This same URL could be put in a requirements.txt file, as is usual for Python developers.

Of course, I won't keep this repository, I just made it for testing tonight. :)

If that patch was merged in pygobject, then Python developers could also install from a release tarball, for example:

  pip install pip install pygobject-3.21.1.tar.xz

This opens the door to installing pygobject in virtual environments.

Created attachment 330281 [details] [review]
Allow installing with pip

This commit adds a setup.py file which just calls the autotools to
configure/make/make install.

It is heavily inspired by the similar work from Simon McVittie on
dbus-python.

In the first patch, I had forgotten to add the new setup.py file to the dist, sorry for all the mail.

One thing missing from this patch is to not hardcode the version in setup.py, but instead take it from configure.ac where it is defined.

Not sure what's the best option here, any idea?

(In reply to Mathieu Bridon from comment #4)
> In the first patch, I had forgotten to add the new setup.py file to the
> dist, sorry for all the mail.
> 
> One thing missing from this patch is to not hardcode the version in
> setup.py, but instead take it from configure.ac where it is defined.
> 
> Not sure what's the best option here, any idea?

Create a setup.py.in that you'll use to generate setup.py, and replace the hardcoded version with @VERSION@.

You might want to check in a generated version of setup.py too, if being able to install from git is planned, otherwise it's fine as-is.

> if being able to install from git is planned,

Installing from git is very much desirable.

In fact, the only reason I created that « fork » on Github is so I could test that it works (see the command I listed in a previous command). I have no desire to actually fork pygobject, this was purely for testing purpose. :)

However, checking in git a generated setup.py file means there is a risk it could get stale, with an old version from back when it was generated.

So I'm not sure that's a good solution. :-/

(In reply to Mathieu Bridon from comment #6)
> > if being able to install from git is planned,
> 
> Installing from git is very much desirable.
> 
> In fact, the only reason I created that « fork » on Github is so I could
> test that it works (see the command I listed in a previous command). I have
> no desire to actually fork pygobject, this was purely for testing purpose. :)
> 
> However, checking in git a generated setup.py file means there is a risk it
> could get stale, with an old version from back when it was generated.

Not if we automate the creation, so that running "make distcheck" will update the file automatically, or even having the release creation fail if the version change in the file isn't checked in.

> So I'm not sure that's a good solution. :-/

Or the hacky way:

import re
with open("configure.ac", "r") as h:
    version = ".".join(re.findall("pygobject_[^\s]+_version,\s*(\d+)\)", h.read()))

(In reply to Christoph Reiter (lazka) from comment #8)
> Or the hacky way:
> 
> import re
> with open("configure.ac", "r") as h:
>     version = ".".join(re.findall("pygobject_[^\s]+_version,\s*(\d+)\)",
> h.read()))

That could work, I don't know how exactly pip downloads and unpacks things. Mathieu, want to try that out?

(In reply to Bastien Nocera from comment #9)
> I don't know how exactly pip downloads and unpacks things.

It gets the sources (download+extract tarball, clone git, ...) then simply runs the `setup.py` script passing it the `install` argument.

(In reply to Christoph Reiter (lazka) from comment #8)
> Or the hacky way:
> 
> import re
> with open("configure.ac", "r") as h:
>     version = ".".join(re.findall("pygobject_[^\s]+_version,\s*(\d+)\)",
> h.read()))

That indeed works just fine.

The reason I didn't do anything like this in the original patch is that there are a number of ways to do it, each more hacky than the other, and with different trade-offs, so I wanted to start a discussion on what upstream would prefer. :)

This one is a bit fragile, in that if the configure.ac syntax to declare the version changes (new order, new variable names, ...) then getting the version in this way will get something entirely incorrect and silently proceed with the installation.

Not sure that's too big a problem though. What do you think?

(In reply to Mathieu Bridon from comment #10)
> (In reply to Bastien Nocera from comment #9)
> > I don't know how exactly pip downloads and unpacks things.
> 
> It gets the sources (download+extract tarball, clone git, ...) then simply
> runs the `setup.py` script passing it the `install` argument.
> 
> (In reply to Christoph Reiter (lazka) from comment #8)
> > Or the hacky way:
> > 
> > import re
> > with open("configure.ac", "r") as h:
> >     version = ".".join(re.findall("pygobject_[^\s]+_version,\s*(\d+)\)",
> > h.read()))
> 
> That indeed works just fine.
> 
> The reason I didn't do anything like this in the original patch is that
> there are a number of ways to do it, each more hacky than the other, and
> with different trade-offs, so I wanted to start a discussion on what
> upstream would prefer. :)
> 
> This one is a bit fragile, in that if the configure.ac syntax to declare the
> version changes (new order, new variable names, ...) then getting the
> version in this way will get something entirely incorrect and silently
> proceed with the installation.
> 
> Not sure that's too big a problem though. What do you think?

Add a "check:" target in the Makefile.am that would fail if the version set by configure.ac isn't the same as computed by that above code?

Review of attachment 330281 [details] [review]:

doing setup.py build install creates "build/", "dist/", "pygobject.egg-info/". These should be added to .gitignore.

Otherwise works as advertised.

::: setup.py
@@ +1,1 @@
+import os

Making it executable and adding a shebang would be nice

@@ +10,3 @@
+
+# TODO: Get this from configure.ac
+version = '3.21.1'

See previous discussion

@@ +20,3 @@
+        srcdir = os.getcwd()
+        builddir = os.path.join(srcdir, self.build_temp)
+        os.makedirs(builddir, exist_ok=True)

This fails with python2 here

Created attachment 330540 [details] [review]
Allow installing with pip

This commit adds a setup.py file which just calls the autotools to
configure/make/make install.

It is heavily inspired by the similar work from Simon McVittie on
dbus-python.

Created attachment 330541 [details] [review]
Allow installing with pip

This commit adds a setup.py file which just calls the autotools to
configure/make/make install.

It is heavily inspired by the similar work from Simon McVittie on
dbus-python.

Thanks for the review Christoph, the patch I just attached fixes the issues you mentioned.

For the version, I went with your regex-based solution.

As we discussed on IRC, let's not worry too much about the relative fragility of these 2 lines. (it's not very likely that configure.ac would change how the version is declared, and we can fix it if it ever does)

Review of attachment 330540 [details] [review]:

::: Makefile.am
@@ +25,3 @@
 	m4/jhflags.m4 \
+	m4/python.m4 \
+	setup.py

This is missing a "check" target, something which would look like:

check:
        $(AM_V_GEN) $(builddir)/test-version.py

Modify configure.ac to have test-version.py in the AC_OUTPUT.
And add a test-version.py.in which has something like:

#!/bin/python

import re
	with open("configure.ac", "r") as h:
	    version = ".".join(re.findall("pygobject_[^\s]+_version,\s*(\d+)\)", h.read()))

if version != @VERSION@:
      print (stderr, "Versions do not match, setup.py needs updating")
      sys.exit(1)

(I'll claim that's pseudo-code, it's just me not being used to Python)

(In reply to Bastien Nocera from comment #16)
> Review of attachment 330540 [details] [review] [review]:
<snip>
> check:

Make that:
check: test-version.py configure.ac

>         $(AM_V_GEN) $(builddir)/test-version.py

Rest still applies.

Review of attachment 330541 [details] [review]:

::: setup.py
@@ +30,3 @@
+        raise
+os._real_makedirs = os.makedirs
+os.makedirs = makedirs

Please don't monkey patch os.makedirs that way. This would break any code passing a mode to makedirs and we don't know if distutils/setuptools might do that.
Just call your custom makedirs below.

(In reply to Bastien Nocera from comment #16)
> Review of attachment 330540 [details] [review] [review]:
> 
> ::: Makefile.am
> @@ +25,3 @@
>  	m4/jhflags.m4 \
> +	m4/python.m4 \
> +	setup.py
> 
> This is missing a "check" target, something which would look like:

We discussed on IRC and decided to leave that for now. I hope that's OK.

Created attachment 330554 [details] [review]
Allow installing with pip

This commit adds a setup.py file which just calls the autotools to
configure/make/make install.

It is heavily inspired by the similar work from Simon McVittie on
dbus-python.

Anything else I can do with this patch?

Review of attachment 330554 [details] [review]:

lgtm

Attachment 330554 [details] pushed as f65bb1f - Allow installing with pip

I've added a setuptools based setup.py in bug 789211. Posting here in case anyone would like to add anything there.