506845 - targets are not rebuilt when archives from EXPAND_LIBNAME change

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Karl Tomlinson (:karlt)]

With --enable-application=browser:

% make -f client.mk build
% touch obj/memory/jemalloc/libjemalloc.a
% make -C obj/browser/app

Expected results:
obj/browser/app/firefox-bin newer than obj/memory/jemalloc/libjemalloc.a.

Actual results:
obj/browser/app/firefox-bin older than obj/memory/jemalloc/libjemalloc.a.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: patch

Using a filename rather than -Ldir -lname allows LIBS_DEPS from
config/rules.mk to work.

Comment 2

[Benjamin Smedberg]

Hrm... is this in particular because jemalloc is a static library? If not, we should add all -lfoo to LIBS_DEPS as well (gmake searches for -lfoo dependencies correctly). We use this pattern a lot...

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to comment #2)
> Hrm... is this in particular because jemalloc is a static library?

Yes.  The lack of a dependency is always a problem for static libraries.

For shared libraries, it is not usually an issue.

> If not, we should add all -lfoo to LIBS_DEPS as well

Maybe would should anyway, if we are being thorough, though it is less important.
It may be useful to rebuild objects depending on shared libraries even to check that symbols removed from the shared libraries are not needed, for example.

> (gmake searches for -lfoo dependencies correctly).

I had forgotten that, but I'm not sure how to deal with -Lpath.

http://www.gnu.org/software/automake/manual/make/Libraries_002fSearch.html

Should we add this to rules.mk?:

vpath $(LIB_PREFIX)%.$(LIB_SUFFIX) \
  $(patsubst -L%,%,$(filter -L%, $(LIBS) $(HOST_LIBS) $(EXTRA_DSO_LDOPTS)))

What is meant by "import libs" here?

http://hg.mozilla.org/mozilla-central/file/74962214e308/config/rules.mk#l106

What might be the problem with always expanding
$(call EXPAND_LIBNAME_PATH,foo,dir)
to dir/$(LIB_PREFIX)foo.$(LIB_SUFFIX)?

Readability I s'pose.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to comment #3)
> vpath $(LIB_PREFIX)%.$(LIB_SUFFIX) \
>   $(patsubst -L%,%,$(filter -L%, $(LIBS) $(HOST_LIBS) $(EXTRA_DSO_LDOPTS)))

And another for DLL_PREFIX/SUFFIX.

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to comment #3)
> What might be the problem with always expanding
> $(call EXPAND_LIBNAME_PATH,foo,dir)
> to dir/$(LIB_PREFIX)foo.$(LIB_SUFFIX)?

I guess we'd need to be specific with a EXPAND_DLLNAME_PATH.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 391001 [details] [diff] [review]
patch

EXPAND_LIBNAME_PATH is also being used for compression and image static
libraries, so we need to fix that too.

Comment 7

[Karl Tomlinson (:karlt)]

EXPAND_LIBNAME_PATH is being used for both static and shared libraries.

I don't think shared libraries are really need to be dependencies in practice.
If the exported symbols change, there will probably be a header that changes
and causes the targets to be rebuilt, which is enough if the library is
rebuilt first.

However, it could be inconvenient to have to use a different macro depending
on whether the library is built static or shared.

I think I have enough of an understanding re import libs now.  The
$(path)/$(LIB_PREFIX)$(lib).$(LIB_SUFFIX) works on the platforms on which we
set _LIBNAME_RELATIVE_PATHS on because $(LIB_SUFFIX) is the same as
$(IMPORT_LIB_SUFFIX) and $(LIB_PREFIX) is the same as $(DLL_PREFIX).

I'll try out setting vpath.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: add libs to _DEPS and set up vpaths

The search path that make uses is not the same as the search path that gcc
uses so this doesn't try to generate deps from OS_LIBS.  (We'd need to start
using gcc --print-search-dirs or gcc --print-file-name, I think.)
I don't know the precise purpose of EXTRA_LIBS, but there doesn't seem to be
an EXTRA_LIBS_DEPS so deps are not generated from EXTRA_LIBS either.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 391561 [details] [diff] [review]
add libs to _DEPS and set up vpaths

The semantics of vpath with one argument (only a pattern) is a little different and not what we want here.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: add libs to _DEPS and set up vpaths v2 [Checkin: Comment 12]

Check that there are some directories before using vpath.

Comment 11

[Benjamin Smedberg]

Comment on attachment 391703 [details] [diff] [review]
add libs to _DEPS and set up vpaths v2
[Checkin: Comment 12]

Assuming this passed tryserver, looks good.

Comment 12

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/8fa93262b47b

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-CC) Copy it to comm-central [Checkin: Comment 14]

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 428607 [details] [diff] [review]
(Bv1-CC) Copy it to comm-central
[Checkin: Comment 14]


http://hg.mozilla.org/comm-central/rev/05a786323772