make install rewrites source files

make install rewrites source files

[Hallvard Breien Furuseth]

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>.


Index: INSTALL
--- INSTALL~
+++ INSTALL
@@ -29,6 +29,6 @@ If you're willing to trade off (much) lo
 faster git you can also do a profile feedback build with
 
-	$ make profile-all
-	# make prefix=... install
+	$ make profile-all     prefix=...
+	# make profile-install prefix=...
 
 This will run the complete test suite as training workload and then
Index: Makefile
--- Makefile~	2012-01-19 01:36:02.000000000 +0100
+++ Makefile	2012-01-23 14:44:56.554980323 +0100
@@ -2695,5 +2695,5 @@ cover_db_html: cover_db
 ### profile feedback build
 #
-.PHONY: profile-all profile-clean
+.PHONY: profile-all profile-clean profile-install
 
 PROFILE_GEN_CFLAGS := $(CFLAGS) -fprofile-generate -DNO_NORETURN=1
@@ -2708,2 +2708,5 @@ profile-all: profile-clean
 	$(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" -j1 test
 	$(MAKE) CFLAGS="$(PROFILE_USE_CFLAGS)" all
+
+profile-install:
+	$(MAKE) CFLAGS="$(PROFILE_USE_CFLAGS)" install

Re: make install rewrites source files

[Junio C Hamano]

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

> INSTALL says we can install a profiled Git with
> 	$ make profile-all
> 	# make install prefix=...
> This does not work...

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'?

Re: make install rewrites source files

[Hallvard Breien Furuseth]

On Mon, 23 Jan 2012 12:15:07 -0800, Junio C Hamano <gitster@pobox.com> wrote:
> Hallvard Breien Furuseth <h.b.furuseth@usit.uio.no> writes:
> 
>> INSTALL says we can install a profiled Git with
>> 	   $ make profile-all
>> 	   # make install prefix=...
>> This does not work...
> 
> 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.

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

> 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'?

'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

-- 
Hallvard

Re: make install rewrites source files

[Clemens Buchacher]

On Mon, Jan 23, 2012 at 09:57:51PM +0100, Hallvard Breien Furuseth wrote:
> 
> 'profile-all' makes 'all' with different CFLAGS from those in
> Makefile.

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

Re: make install rewrites source files

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> 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

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

Re: make install rewrites source files

[Hallvard Breien Furuseth]

On Thu, 26 Jan 2012 23:52:31 +0100, Clemens Buchacher <drizzd@aon.at> wrote:
> How about removing the profile-all target and making it a build option
> instead? To enable it, do the usual:
> (...)
> 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'.

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.

-- 
Hallvard

Re: make install rewrites source files

[Phillip Susi]

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

On 1/23/2012 9:18 AM, Hallvard Breien Furuseth wrote:
> INSTALL says we can install a profiled Git with $ make profile-all 
> # make install prefix=...

prefix should be an argument to configure, not make.

> 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.

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

> 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>.

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

> Index: Makefile --- Makefile~	2012-01-19 01:36:02.000000000 +0100 
> +++ Makefile	2012-01-23 14:44:56.554980323 +0100

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-----

Re: make install rewrites source files

[Junio C Hamano]

Phillip Susi <psusi@ubuntu.com> writes:

> On 1/23/2012 9:18 AM, Hallvard Breien Furuseth wrote:
>> INSTALL says we can install a profiled Git with $ make profile-all 
>> # make install prefix=...
>
> prefix should be an argument to configure, not make.

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

Re: make install rewrites source files

[Hallvard B Furuseth]

 On Mon, 23 Jan 2012 15:28:39 -0500, Phillip Susi <psusi@ubuntu.com> 
 wrote:
> On 1/23/2012 9:18 AM, Hallvard Breien Furuseth wrote:
>> 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>.
>
> 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
> (...)

 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.

-- 
 Hallvard