Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#2872 closed defect (fixed)

make install builds documentation

Reported by: gdt Owned by: strk
Priority: medium Milestone: PostGIS 2.1.4
Component: build/upgrade/install Version: 2.1.x
Keywords: history Cc:

Description

make should perform all building steps, and make install should only copy (via "install") files to ${DESTDIR}, with an exception for libtool to relink binaries/libraries that had been pointing at libraries in the workding directory.

To repeat, run make as a non-root user, and make install as root, and look for root-written files in the working directory, as well as observing build commands in the log.

This isn't incredibly serious, but could be cause for packaging systems to reject postgis.

I definitely just saw this, but now can't reproduce. Anyone else?

Attachments (1)

0001-Simplify-docs-building-rules-hopefully-fixing-build-.patch (3.9 KB) - added by strk 5 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by gdt

I found that "make install install-docs" invoked:

/usr/pkg/bin/xsltproc --novalid ./xsl/postgis_comments.sql.xsl postgis-out.xml > postgis_comments.sql
/usr/pkg/bin/xsltproc ./xsl/raster_comments.sql.xsl postgis-out.xml > raster_comments.sql
/usr/pkg/bin/xsltproc --novalid ./xsl/topology_comments.sql.xsl postgis-out.xml > topology_comments.sql
/usr/pkg/bin/xsltproc --novalid ./xsl/tiger_geocoder_comments.sql.xsl postgis-out.xml > tiger_geocoder_comments.sql

comment:2 Changed 5 years ago by gdt

Version: 2.0.x2.1.x

Also, the topology extension creates files during install:

for OLD_VERSION in 2.0.0 2.0.1 2.0.2 2.0.3 2.0.4 2.0.5 2.1.0rc1 2.1.0rc2 2.1.0rc3 2.1.0 2.1.1 2.1.2; do \
          cat ../postgis_extension_helper.sql sql_bits/remove_from_extension.sql.in sql/topology_upgrade_minor.sql sql_bits/mark_editable_objects.sql.in sql_bits/topology_comments.sql ../postgis_extension_helper_uninstall.sql > sql/postgis_topology--$OLD_VERSION--2.1.3.sql; \
done

This happens for "gmake all", which is fine, but again for "gmake install". Confirmed with 2.1.3, NetBSD 6 i386, postgresql 9.1

comment:3 Changed 5 years ago by strk

Can you check with --disable-extension ? I belive the problem is with extension requiring comments which require DocBook? and is thus not built at "make" time, or something like that.

The correct solution would be to always build docs at "make" time if deps are met, or warn and disable extension if deps are not met.

comment:4 Changed 5 years ago by gdt

--disable-extension is unrecogized. With --without-topology --without-raster, I find that the xsltproc are unchanged, but the comment:2 for/cat loop is no longer present.

Apparently with pkgsrc moving to 2.1.3 there is now also

/usr/pkg/bin/xsltproc --novalid ./xsl/sfcgal_comments.sql.xsl postgis-out.xml > sfcgal_comments.sql

It's a bit hard to follow the makefiles, but it seems the top-level makefile install target invokes comments-install, so the xsltproc doesn't seem to be about extensions, which matches what I'm seeing.

comment:5 Changed 5 years ago by strk

There seems to be a different handling of dependencies between "all" and "install" rules, the "all" rule wants XSLTPROC, XSLBASE and IMAGEMAGICK while the "install" rule only wants XSLTPROC. Could this be the source of the problem ?

comment:6 Changed 5 years ago by strk

NOTE: the core extension is not disabled by any of those --without calls. I'm afraid we have indeed no option to disable extension building.

comment:7 Changed 5 years ago by strk

Still, if you only run "make" and "make install" from within the doc/ subdir there should be no influence from the extension mechanism.

Ideally "make install" from doc/ dir should not build anything new from "make" within the same dir.

comment:8 Changed 5 years ago by gdt

I found a bug. In GNUMakefile.in, it checks for $(XSLTPROC). But this is never set. It should be @XSLTPROPC@, so that it is substituted. Or there should be XSLTPROC=@XSLTPROC@ above.

I also don't understand why comments are in the doc directory, since they seem to be something different.

Also, if this wrapping stays, the install target should have the same wrapping of ifdef @XSLTPROC@.

(And, I don't really understand why docs is a separate build target, or rather why the all: target doesn't include it. The straightforward case is to have all the build dependencies and to just run make && make install. I can certainly see having a code-only target for people without docbook, but that seems like an exceptional situation rather than normal.)

comment:9 Changed 5 years ago by strk

Good catch. That missing @XSLTPROC@ was my fault, introduced in r9887 to fix #1779

comment:10 Changed 5 years ago by gdt

So perhaps we should split this bug, unless you can also fix the cat stuff easily.

comment:11 Changed 5 years ago by strk

See if the attached patch fixes the problem for you. It's been produced in trunk.

comment:12 Changed 5 years ago by strk

@gdt, did you have a chance to test my patch ?

comment:13 Changed 5 years ago by strk

I've committed the patch in trunk with r12894 -- if it helps I'd backport to 2.1

comment:14 Changed 5 years ago by strk

@gdt any news on this ?

comment:15 Changed 5 years ago by gdt

(Sorry for the delay.) I applied the patch, and did build/install with and without the patch. The patch didn't seem to cause any problems, and it fixed a lot of the build activity at install time problems. So I'd say yes, please backport to 2.1.

I do see some remaining issues, basically the 2 for loops to create upgrade files for various versions. The code does get run during build, but is rerun at install time, as shown by "make install" output and also new timestamps in the source tree.

for OLD_VERSION in 2.0.0 2.0.1 2.0.2 2.0.3 2.0.4 2.0.5; do \
          cat sql_bits/extension_upgrade_minor.sql > sql/postgis--$OLD_VERSION--2.1.3.sql; \
done
for OLD_VERSION in 2.1.0rc1 2.1.0rc2 2.1.0rc3 2.1.0 2.1.1 2.1.2; do \
          cat sql_bits/extension_upgrade_patch.sql > sql/postgis--$OLD_VERSION--2.1.3.sql; \
done

I can't immediately explain, but it looks like the make rule always fires and then runs a for loop, without some sort of "touch upgrade-multiversion-done" canary. Either that or explicit rules per version, but that sounds really awkward.

comment:16 Changed 5 years ago by strk

Thanks for testing, backported to 2.1 branch (for 2.1.4) with r12922

Please file a new ticket for the 2 loops. It's due to "sql_minor_upgrade" and "sql_patch_upgrade" targets being phony (implicitly)

comment:17 Changed 5 years ago by gdt

Resolution: fixed
Status: newclosed

comment:18 Changed 5 years ago by robe

Keywords: history added
Note: See TracTickets for help on using tickets.