395019 - compute relativesrcdir from srcdir, topsrcdir

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

relativesrcdir gets set in a lot of Makefiles.  It seems like we could just compute it, perhaps in autoconf.mk, like so:

relativesrcdir = $(subst $(topsrcdir)/,,$(srcdir))

We'll need to test this and make sure it works consistently.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: simple fix in autoconf.mk.in [Backed out: Comment 7]

I tested this on MSYS, Cygwin, Linux, and OSX, and it seems to work fine on all of them.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 280387 [details] [diff] [review]
simple fix in autoconf.mk.in
[Backed out: Comment 7]

a=bzbarsky

Can we start removing existing sets of this, then?

Comment 3

[(not currently active) Ted Mielczarek]

Yes.  I didn't think it was worth bloating up this tiny patch for that, since the existing ones shouldn't hurt anything, but we could take them out all at once or in pieces, whichever works.

Checked in.

Comment 4

[Ginn Chen]

It caused this error on Linux and Solaris.

gmake[5]: Entering directory `/export/home/mrbld/tinderbox/SunOS_5.11_Depend/mozilla/netwerk/locales'
error: file '../.././en-US/necko.properties' doesn't exist at ../../config/make-jars.pl line 462, <STDIN> line 4.
+++ making chrome /export/home/mrbld/tinderbox/SunOS_5.11_Depend/mozilla/netwerk/locales  => ../../dist/bin/chrome/en-US.jar
gmake[5]: *** [libs] Error 2
gmake[5]: Leaving directory `/export/home/mrbld/tinderbox/SunOS_5.11_Depend/mozilla/netwerk/locales'
gmake[4]: *** [libs] Error 2

Comment 5

[Ginn Chen]

on my box
netwerk/locales/Makefile has
topsrcdir = ../..
srcdir = .
so
relativesrcdir = $(subst $(topsrcdir)/,,$(srcdir))
i.e. relativesrcdir = .

config.mk Line 864 will not work
because it tries $(topsrcdir)/$(relativesrcdir)/en-US as ../.././en-US

same problem at dom/locales

Reopening this bug, since the computation is not correct.

Comment 6

[[e-mail]]

It seems to me that make-makefile could be used to substitute this in.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 280387 [details] [diff] [review]
simple fix in autoconf.mk.in
[Backed out: Comment 7]

Alright, I backed this out for now.  Ginn: what does your build environment look like?  Do you have an objdir set in your mozconfig?

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to comment #6)
> It seems to me that make-makefile could be used to substitute this in.

That would certainly be doable, but I was hoping for a simpler solution.  I guess it might not be possible.

Comment 9

[(not currently active) Ted Mielczarek]

FWIW, I think this is only broken for srcdir builds, unless someone knows of another case where this can happen.

Comment 10

[Ginn Chen]

Yes, I'm building in srcdir.

The following line works for me. But I didn't test on any other platform, or configure/build in another dir.

relativesrcdir = $(subst $(shell cd $(topsrcdir); pwd)/,,$(shell cd $(srcdir); pwd))

Comment 11

[Stuart Parmenter]

Comment on attachment 280387 [details] [diff] [review]
simple fix in autoconf.mk.in
[Backed out: Comment 7]

clearing approval on obsolete patch.  please ask again for a new patch

Comment 12

[Serge Gautherie (:sgautherie)]

When the autoconf.mk part is fixed (here),
I can do the cleanup part (while doing bug 479998) ;-)

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: Non-matching |relativesrcdir| value list

There are 2 possibilities to fix these 4 occurrences:
1)
Move the |relativesrcdir| setting after autoconf.mk inclusion to override the global definition.
Less work, but leaves exceptions.
2)
Adapt them to match the expected value/location.
More work, but no exceptions left.

The choice is yours.

*****

(In reply to comment #10)
> relativesrcdir = $(subst $(shell cd $(topsrcdir); pwd)/,,$(shell cd $(srcdir);
> pwd))

Do we want to do it exactly like this ?
Or is calling the shell (twice) considered too expansive and we would want to try the simple substitution first and only do this as a fallback?

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: Non-matching |relativesrcdir| value list

The complete one, this time!


It looks like all the tests could be fixed to match the new global rule.
(with the possibility to use an override if some can't be adapted.)

To be safe with the locales, I would suggest to rename the variable they use to |localerelativesrcdir| for example.

What/How do you think we should do?

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> > relativesrcdir = $(subst $(shell cd $(topsrcdir); pwd)/,,$(shell cd $(srcdir);
> > pwd))
> 
> Do we want to do it exactly like this ?
> Or is calling the shell (twice) considered too expansive and we would want to
> try the simple substitution first and only do this as a fallback?

(In reply to comment #14)
> To be safe with the locales, I would suggest to rename the variable they use to
> |localerelativesrcdir| for example.
> 
> What/How do you think we should do?

Ted, how should we proceed on this bug?

Comment 16

[(not currently active) Ted Mielczarek]

We're not putting two shell calls in every makefile just to fix this. I think the only safe way to fix this is to make make-makefile know how to produce this, and then start sticking "relativesrcdir = @relativesrcdir@" in makefiles.

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)

(1.5 years later:)

> on my box

I tried on my local W2K with and without |mk_add_options MOZ_OBJDIR=...| and got the same full paths in both cases...

> netwerk/locales/Makefile has
> topsrcdir = ../..
> srcdir = .

topsrcdir	= [...]/mozilla
srcdir		= [...]/mozilla/netwerk/locales

> same problem at dom/locales

topsrcdir      = [...]/mozilla
srcdir         = [...]/mozilla/dom/locales

I asked Ted who replied "it's possible something changed".

Ginn, or anyone, would you know how to reproduce the '. and ..' output?
(NB: make-makefile seems to still have code to handle "dots"...)

Otherwise, I would suggest to check Ted's patch in again.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> Non-matching |relativesrcdir| value list

Is the following a problem or not ?
{
825 ifdef relativesrcdir
826 LOCALE_SRCDIR = $(call EXPAND_LOCALE_SRCDIR,$(relativesrcdir))
827 endif
}

> To be safe with the locales, I would suggest to rename the variable they use to
> |localerelativesrcdir| for example.

Ted, what about these two questions?

Comment 19

[Serge Gautherie (:sgautherie)]

3 years without answers, giving up.

Comment 20

[Joey Armstrong [:joey]]

Attachment: compute relativesrcdir from srcdir, topsrcdir

Comment 21

[Joey Armstrong [:joey]]

Comment on attachment 632703 [details] [diff] [review]
compute relativesrcdir from srcdir, topsrcdir

Added makefile library function getRelSrcDir able to compute paths from topsrcdir and srcdir.

Usage: relpath = $(call getRelSrcDir)

Function will verify topsrcdir and srcdir both contain a value.
$(relpath ) is used to normalize paths (~symlinks).
$(or ) used to detect non-existent/yet to be created paths.

Unit test will verify subdir extraction from:
  o absolute paths
  o ../.., ./
  o non-existent, non-overlapping paths

Comment 22

[Mike Hommey [:glandium]]

Note that realpath requires make 3.81 and we currently require 3.80.

Comment 23

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #22)
> Note that realpath requires make 3.81 and we currently require 3.80.

3.81 was released August 1, 2006. Just saying ;)

Comment 24

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #23)
> (In reply to Mike Hommey [:glandium] from comment #22)
> > Note that realpath requires make 3.81 and we currently require 3.80.
> 
> 3.81 was released August 1, 2006. Just saying ;)

Do we still have platforms dependent on v3.80 or has the minimum requirement just not been updated for a while ?  Even make bundled with mozilla-build(~msvc9) appears to have been patched to a newer version:

$ make --version
GNU Make 3.81.90

This program built for i686-pc-msys32

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 632703 [details] [diff] [review]
compute relativesrcdir from srcdir, topsrcdir

Review of attachment 632703 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I ran out of time trying to get reviews done today. Feel free to punt this to someone else or I can look at it when I'm back from vacation.

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 632703 [details] [diff] [review]
compute relativesrcdir from srcdir, topsrcdir

glandium's mega-patch-series in bug 774032 is taking care of this in a different manner, which is probably the better way to go in the long run.