make install rewrites source files

INSTALL says we can install a profiled Git with
	$ make profile-all
	# make install prefix=...
This does not work: 'make install' notices that the build flags has
changed and rebuilds Git - presumably without using the profile info.
The patch below fixes this.

However, make install should not write to the source directory in any
case.  That fails as root if root lacks write access there, due to NFS
mounts that map root to nobody etc.  At least git-instaweb and
GIT-BUILD-OPTIONS are rewritten.  You can simulate this with
    su nobody -s /bin/bash -c 'make -k install'
after configuring with prefix=<directory owned by nobody>.

Hallvard Breien Furuseth <h.b.furuseth@usit.uio.no> writes:

We should just drop prefix=... from that line, as the "prefix=value must be the same while building and installing" is not only about the "profile" build but applies to any other build.

I however wonder why you would need a separate profile-install target, though. Shouldn't

	$ make foo-build && make install

install a funky 'foo' variant of the build for any supported value of 'foo'?

On Mon, 23 Jan 2012 12:15:07 -0800, Junio C Hamano <gitster@pobox.com> wrote:

Either add or remove a prefix so they match, yes.  Fine by me either way.

'profile-all' makes 'all' with different CFLAGS from those in
Makefile.  'install' makes 'all' which notices CFLAGS has changed
since last build, so it rebuilds:
    $ make install
    * new build flags or prefix
    ...
That's also how the 2nd '$(MAKE) ... all' in profile-all can tell
that it should do anything.  Thus my new 'profile-install:' target
with the same flags as the final $(MAKE) in profile-all.

This looks way too clever to me. 'make' can detect that flags have changed, but should then fail (optionally?) instead of rebuilding. That'd likely solve my issue with other files rewritten as root too. But I'm not volunteering to rewrite your build system.

BTW, it'd be useful to split up 'profile-all' so it is possible to ignore 'make test' failure and compilete the build anyway:

.PHONY: profile-all profile-clean profile-gen profile-use profile-install
profile-all: profile-clean profile-gen profile-use
profile-gen:
	$(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" all
	$(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" -j1 test
profile-use:
	$(MAKE) CFLAGS="$(PROFILE_USE_CFLAGS)" all

On Mon, Jan 23, 2012 at 09:57:51PM +0100, Hallvard Breien Furuseth wrote:

How about removing the profile-all target and making it a build option instead? To enable it, do the usual:

 echo PROFILE_BUILD=YesPlease >> config.mak
 echo prefix=... >> config.mak
 make
 su make install

In the Makefile, we would have

ifdef PROFILE_BUILD
 all:
	$(MAKE) CFLAGS=... -fprofile-generate ... all-one
	$(MAKE) CFLAGS=... -fprofile-use ... all-one
else
 all: all-one
endif

and each previous instance of 'all' replaced with 'all-one'.

Clemens

Clemens Buchacher <drizzd@aon.at> writes:

Yeah, I would prefer something like that. We could even keep "profile-all" target for b/c if we wanted to, no?

On Thu, 26 Jan 2012 23:52:31 +0100, Clemens Buchacher <drizzd@aon.at> wrote:

Not quite. test: and install: should depend on 'all', otherwise making them without doing 'make all' first will test/install an unprofiled Git.

So 'all' with profiling should be today's profile-all, which should not throw away the build and start over. It can create some files to mark how far it has gotten instead. And profile-generate currently uses 'test' which would recurse, it needs another internal test target.

Not sure if it is worth it. Something like this, perhaps. Except I have not thought about how this interacts with the coverage targets.

# Final targets

ifdef PROFILE_BUILD all:: profile-all test: profile-test install: profile-install else all:: all-one test: test-one install: install-one endif

# Profiling # # Note: If profiling (the test phase) failed halfway through but you # still want to use the partial profile results to build Git, you can # touch p-gen.stamp # and then 'make all' again.

profile-all: p-use.stamp

profile-gen p-gen.stamp:
	$(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" all-one
	$(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" -j1 test-one
	touch p-gen.stamp

profile-use p-use.stamp: p-gen.stamp
	$(MAKE) CFLAGS="$(PROFILE_USE_CFLAGS)" all-one
	touch p-use.stamp

profile-test: p-use.stamp
	$(MAKE) CFLAGS="$(PROFILE_USE_CFLAGS)" test-one

profile-install: p-use.stamp
	$(MAKE) CFLAGS="$(PROFILE_USE_CFLAGS)" install-one

.PHONY: all-one test test-one install install-one .PHONY: profile-all profile-gen profile-test profile-install profile-clean

Also let 'clean' depend on 'profile-clean' which does
	$(RM) p-gen.stamp p-use.stamp.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/23/2012 9:18 AM, Hallvard Breien Furuseth wrote:

prefix should be an argument to configure, not make.

make install implicitly includes make all; it is supposed to rebuild anything that needs rebuilt.

If you want to build locally from a read only nfs mount, then you should run the configure script in a local directory:

mkdir /tmp/build cd /tmp/build /path/to/nfs/source/configure make make install

Hrm... Makefile should itself be a generated file from Makefile.in or Makefile.am, but it appears that git isn't doing this. Perhaps that should be fixed.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPHcL3AAoJEJrBOlT6nu759U8IANCtJDWnCizSDWrJAFWe3ISr FemiFgW347qjLcWlJS036nPfKnrxrJ88rF2e9+8Tj/hfPojNwCmyvN7rz+guI0uA qqOfk9uN38Qd/jwfW5gv/7raKP4eUyRZ9ioptX3NqQtP5Co4TFuajOfswpN8f/DL QiU7os62Df5HWW2U8A3XT9KiU9oWRala8dcrp5EJkEOfYDvQG2o3e1N/D91KC4el lAyVEnzrvoLr5NzHCnFe7dQqvAB2S3PE/NP4anZHyNRp3SDLu1iZbD9MKC21Bd3n BmCn9Vh7U+reC/NBMq8qaM69jLRk2Dx12brFoyY5/cjdQuaLj+n6h1nN9MqSKYI= =/qLy -----END PGP SIGNATURE-----

Phillip Susi <psusi@ubuntu.com> writes:

What are you talking about? Use of ./configure is entirely optional, and our Makefile _does_ support giving prefix on the command line.

 On Mon, 23 Jan 2012 15:28:39 -0500, Phillip Susi <psusi@ubuntu.com> 
 wrote:

 Not a read-only nfs mount.  Just an ordinary remote mount where root
 on the local host is mapped to nobody on the remote host.  (Having
 local root access does not mean you should get root on the remote.)

 In any case, it's normal practice to do as little as possible as root,
 and also to at least try not write to the source dir during install.
 BTW, building in /tmp can be nasty to other users when you don't know
 how much space the build (and maybe test) will use, so you may need
 access to some other local dir.