Fix gdal installing Python modules for default interpreter #2423

cjmayo · 2016-09-27T19:19:20Z

gdal with USE="python" is installing Python modules for the default interpreter even if that version of Python is not selected in PYTHON_TARGETS because --with-python is set in first call of gdal_src_configure from src_configure (before the if use python).
Therefore control Python installation with $use_python.

Also a commit to byte-compile Python modules.

@Whissi

mgorny

While you don't seem to actually make anything worse, the ebuild is of terrible quality and I can't approve this as-is.

As far as I can see, this is yet another terrible example of Gentoo developers overhacking simple things. Do you wish to work on it more to bring some sanity, or should I just open a bug for the current maintainer to resolve it?

mgorny · 2016-10-05T12:12:15Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+PYTHON_COMPAT=( python2_7 python3_{3,4} )
+DISTUTILS_OPTIONAL=1
+
+inherit autotools eutils libtool perl-module distutils-r1 python-r1 toolchain-funcs java-pkg-opt-2


This ebuild is not using distutils-r1 at all… (I know you haven't touched it but it's a major problem and since you are already touching Python stuffs, it would be nice to have this fixed)

Disregard this. We'll make it used.

mgorny · 2016-10-05T12:14:11Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+REQUIRED_USE="
+	spatialite? ( sqlite )
+	mdb? ( java )
+"


python? ( ${PYTHON_REQUIRED_USE} ) missing

mgorny · 2016-10-05T12:26:27Z

Good news is that the package's build system is so terrible, we don't have to care much about it and fixing it won't take much time. I'll provide proper instructions in an hour so.

mgorny

Here's a quick summary of changes needed to be done, looking at the build system. Long story short, don't use configure at all, use make to generate swig files (once for all impls), then let distutils-r1 do the rest.

mgorny · 2016-10-05T12:46:53Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+	sed -i \
+		-e "s:setup.py install:setup.py install --root=\$(DESTDIR):" \
+		-e "s:--prefix=\$(DESTDIR):--prefix=:" \
+		"${S}"/swig/python/GNUmakefile || die


Kill this sed.

mgorny · 2016-10-05T12:47:22Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+	}
+	if use python; then
+		python_foreach_impl prepare_python
+	fi


Kill the whole Python part here.

mgorny · 2016-10-05T12:47:39Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+		myopts+=" --with-python"
+	else
+		myopts+=" --without-python"
+	fi


Kill this whole thing…

mgorny · 2016-10-05T12:48:31Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+		--without-oci \
+		--without-pcraster \
+		--without-podofo \
+		--without-sde \


…and just put --without-python here. The configure doesn't really care about that switch, only to determine some random useless magic wrt Python and setuptools that actually does more harm than good.

mgorny · 2016-10-05T12:50:20Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+			GDALmake.opt || die "sed LIBS failed"
+	fi
+
+	if [[ -n $use_python ]]; then


This belongs in src_prepare, and you can do it unconditioanlly, it doesn't seem to require anything specific from Python or harm non-Python builds.

mgorny · 2016-10-05T12:52:17Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+
+	compile_python() {
+		rm -f swig/python/*_wrap.cpp || die
+		emake -C swig/python generate


Now, this (the swig part) you can do like for perl -- it's enough to do it once for all impls.

mgorny · 2016-10-05T12:53:55Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+	compile_python() {
+		rm -f swig/python/*_wrap.cpp || die
+		emake -C swig/python generate
+		emake -C swig/python build


The build part do via distutils-r1. Kill the whole foreach thing, and just do sth like:

if use python; then cd "${S}"/swig/perl || die distutils-r1_src_compile fi

(note: looking at other code, maintainer may prefer pushd/popd, don't forget to ||die)

mgorny · 2016-10-05T12:54:45Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+	use doc && dohtml html/*
+
+	install_python() {
+		emake -C swig/python DESTDIR="${D}" install


Again, kill the custom thing and just call distutils-r1 phase function, without foreach.

Needed a pushd, setup.py is not copied to build directory.

-c "${S}/swig/python"? But please make sure that this all works when you uninstall the original package and build it as fresh install (i.e. doesn't accidentally use installed stuff instead of build).

The pushd is because distutils-r1_src_compile works OK for the first Python target but then fails for the second:

x86_64-pc-linux-gnu-g++ -pthread -shared -Wl,-O1 -Wl,--as-needed -Wl,--hash-style=gnu -march=ivybridge -ftree-vectorize -O2 -pipe -fPIC -Wall build/temp.linux-x86_64-3.5/extensions/gdal_array_wrap.o -L../../.libs -L../../ -L/usr/lib64 -L/var/tmp/portage/sci-libs/gdal-2.0.2-r1/work/gdal-2.0.2/lib -lpython3.5m -lgdal -o build/lib.linux-x86_64-3.5/osgeo/_gdal_array.cpython-35m-x86_64-linux-gnu.so make: Leaving directory '/var/tmp/portage/sci-libs/gdal-2.0.2-r1/work/gdal-2.0.2/swig/python' * python2_7: running distutils-r1_run_phase distutils-r1_python_compile /usr/bin/python2.7 setup.py build /usr/bin/python2.7: can't open file 'setup.py': [Errno 2] No such file or directory * ERROR: sci-libs/gdal-2.0.2-r1::newer failed (compile phase):

(and as it shows I am actually testing with Python 3.5)

Your're right about it using installed stuff. It was failing to find gdal-config and falling back silently to the one in /usr/bin.
I don't think the fallback is a great idea, and have filed:
OSGeo/gdal#159

I've moved the default in src_compile() to make sure the gdal-config is built in time.

mgorny · 2016-10-05T12:55:02Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+
+	install_python() {
+		emake -C swig/python DESTDIR="${D}" install
+		python_optimize


This shouldn't be necessary when distutils-r1 installs stuff.

swig/python/GNUmakefile's install has
for f in $(SCRIPTS) ; do $(INSTALL) ./scripts/$$f $(DESTDIR)$(INST_BIN) ; done
where SCRIPTS = ls ./scripts``

which means replacing make install with distutils-r1 causes a failure of:

python_replicate_script "${ED}"/usr/bin/*py

But make install isn't desirable, it installs "man" pages (*.dox files) in /usr/bin/
(suggested fix upstream OSGeo/gdal#157)
and of course is run for each Python target.

So... I've added dobin swig/python/scripts/*.py.

Doesn't setup.py install the necessary scripts? If it would, the replication wouldn't be necessary in the first place.

Unfortunately not.

mgorny · 2016-10-05T12:56:27Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+PYTHON_COMPAT=( python2_7 python3_{3,4} )
+DISTUTILS_OPTIONAL=1
+
+inherit autotools eutils libtool perl-module distutils-r1 python-r1 toolchain-funcs java-pkg-opt-2


Oh, and kill python-r1 then ;-).

mgorny

Thanks for your work so far. Just a few little requests to finish this.

mgorny · 2016-10-09T21:08:12Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+
+	eautoreconf
+
+	prepare_python() {


This function is not used at all.

mgorny · 2016-10-09T21:08:51Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+	if use python; then
+		rm -f "${S}"swig/python/*_wrap.cpp || die
+		emake -C "${S}"/swig/python generate
+		emake -C "${S}"/swig/python build


This only calls setup.py, which distutils-r1 does later in the ebuild. Remove it.

mgorny · 2016-10-09T21:43:43Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+		newdoc swig/python/README.txt README-python.txt
+		insinto /usr/share/${PN}/samples
+		doins swig/python/samples/*
+		dobin swig/python/scripts/*.py


I see that upstream does that in Makefile indeed. Then I suggest you define, before distutils-r1_src_install call:

python_install() { distutils-r1_python_install python_doscript scripts/*.py }

python_install gets called for each impl by distutils-r1_src_install, and distutils-r1_python_install is the default impl that calls setup.py install.

I believe all the comments are addressed now.

mgorny · 2016-10-09T21:44:38Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+		popd > /dev/null
+	fi
+
+	if use python; then


As a side note, I think it'd be more readable to combine the two if python blocks.

mgorny

LGTM modulo pushd||die, popd||die.

mgorny · 2016-10-10T21:11:36Z

sci-libs/gdal/gdal-2.0.2-r1.ebuild

+	if use python; then
+		rm -f "${S}"swig/python/*_wrap.cpp || die
+		emake -C "${S}"/swig/python generate
+		pushd "${S}"/swig/python > /dev/null


All pushd & popd need || die. Feel free to also fix those you haven't added, possibly in a separate commit.

die's added. Not sure about a separate commit as it is a new file anyway - would do the old ones after adding the new stuff?

cjmayo · 2016-10-24T19:13:45Z

Added a fix for jasper slot change. Happy to remove if necessary.

Found when emerging my own qgis ebuild after stable jasper was upgraded from 1.900.1-r9 to :1.900.6

make[2]: Leaving directory '/var/tmp/portage/sci-geosciences/qgis-2.18.0/work/qgis-2.18.0_build'
/usr/lib/gcc/x86_64-pc-linux-gnu/4.9.4/../../../../lib64/libgdal.so: undefined reference to `jp2_encode_uuid'
collect2: error: ld returned 1 exit status

SoapGentoo · 2016-10-31T22:18:04Z

@cjmayo sorry, I had to do an emergency revbump, in order to fix the broken ABI. I added := to all ebuilds, so you might want to rebase on the revbumps instead now.

cjmayo · 2016-11-01T19:35:42Z

No problems (thanks to Git). Renamed to r2 based on r1.

Also adds byte-compilation for Python modules Gentoo-Bug: 536528 Package-Manager: portage-2.3.0

cjmayo · 2016-11-07T19:40:36Z

Renamed to r3 based on r2.

gentoo-repo-qa-bot · 2016-11-25T11:44:29Z

Pull Request assignment

Areas affected: ebuilds
Packages affected: sci-libs/gdal

sci-libs/gdal: @gentoo/sci-geosciences

mgorny · 2016-11-25T11:48:38Z

Re-assigned it correctly. Also @gentoo/python, feel free to merge this if the maintainer doesn't respond since it's mostly a Python-related change (note: diff to be sure).

kensington assigned Whissi Sep 29, 2016

kensington added bugfix assigned PR successfully assigned to the package maintainer(s). labels Sep 29, 2016

mgorny requested changes Oct 5, 2016

View reviewed changes

cjmayo force-pushed the gdal-python branch 2 times, most recently from 48d8df5 to 7f639dc Compare October 9, 2016 18:37

mgorny requested changes Oct 9, 2016

View reviewed changes

cjmayo force-pushed the gdal-python branch from 7f639dc to e878416 Compare October 10, 2016 18:56

mgorny approved these changes Oct 10, 2016

View reviewed changes

cjmayo force-pushed the gdal-python branch from e878416 to 7147928 Compare October 11, 2016 17:31

cjmayo force-pushed the gdal-python branch from 39171c7 to 066f705 Compare November 1, 2016 19:33

sci-libs/gdal: only install Python modules for PYTHON_TARGETS

9310031

Also adds byte-compilation for Python modules Gentoo-Bug: 536528 Package-Manager: portage-2.3.0

cjmayo force-pushed the gdal-python branch from 066f705 to 9310031 Compare November 7, 2016 19:39

mgorny unassigned Whissi Nov 25, 2016

mgorny removed the assigned PR successfully assigned to the package maintainer(s). label Nov 25, 2016

gentoo-repo-qa-bot added the assigned PR successfully assigned to the package maintainer(s). label Nov 25, 2016

gentoo-bot closed this in d35b1a8 Nov 25, 2016

cjmayo deleted the gdal-python branch December 3, 2016 17:45

Whissi mentioned this pull request Dec 4, 2016

sci-libs/gdal: Bump to v2.0.3 #2106

Closed

Fix gdal installing Python modules for default interpreter #2423

Fix gdal installing Python modules for default interpreter #2423

Conversation

cjmayo commented Sep 27, 2016

mgorny left a comment

Choose a reason for hiding this comment

mgorny Oct 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgorny commented Oct 5, 2016

mgorny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjmayo Oct 9, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjmayo Oct 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgorny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgorny left a comment

Choose a reason for hiding this comment

mgorny Oct 10, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjmayo commented Oct 24, 2016

SoapGentoo commented Oct 31, 2016

cjmayo commented Nov 1, 2016

cjmayo commented Nov 7, 2016

gentoo-repo-qa-bot commented Nov 25, 2016

mgorny commented Nov 25, 2016

mgorny Oct 5, 2016 •

edited

cjmayo Oct 9, 2016 •

edited

cjmayo Oct 8, 2016 •

edited

mgorny Oct 10, 2016 •

edited