Opened 8 years ago

Closed 8 years ago

Last modified 8 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 8 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 by gdt, 8 years ago

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 by gdt, 8 years ago

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 by strk, 8 years ago

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 by gdt, 8 years ago

—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 by strk, 8 years ago

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 by strk, 8 years ago

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 by strk, 8 years ago

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 by gdt, 8 years ago

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 by strk, 8 years ago

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

comment:10 by gdt, 8 years ago

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

comment:11 by strk, 8 years ago

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

comment:12 by strk, 8 years ago

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

comment:13 by strk, 8 years ago

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

comment:14 by strk, 8 years ago

@gdt any news on this ?

comment:15 by gdt, 8 years ago

(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 by strk, 8 years ago

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 by gdt, 8 years ago

Resolution: fixed
Status: newclosed

comment:18 by robe, 8 years ago

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