targets are not rebuilt when archives from EXPAND_LIBNAME change

RESOLVED FIXED in mozilla1.9.3a1

Status

defect
RESOLVED FIXED
10 years ago
Last year

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla1.9.3a1
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

10 years ago
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.
Assignee

Comment 1

10 years ago
Posted patch patch (obsolete) — Splinter Review
Using a filename rather than -Ldir -lname allows LIBS_DEPS from
config/rules.mk to work.
Attachment #391001 - Flags: review?(benjamin)

Comment 2

10 years ago
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...
Assignee

Comment 3

10 years ago
(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.
Assignee

Comment 4

10 years ago
(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.
Assignee

Comment 5

10 years ago
(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.
Assignee

Comment 6

10 years ago
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.
Attachment #391001 - Flags: review?(benjamin)
Assignee

Comment 7

10 years ago
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.
Summary: Programs are not rebuilt when jemalloc changes → targets are not rebuilt when archives from EXPAND_LIBNAME change
Assignee

Comment 8

10 years ago
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.
Attachment #391001 - Attachment is obsolete: true
Attachment #391561 - Flags: review?(benjamin)
Assignee

Comment 9

10 years ago
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.
Attachment #391561 - Attachment is obsolete: true
Attachment #391561 - Flags: review?(benjamin)
Assignee

Comment 10

10 years ago
Check that there are some directories before using vpath.
Assignee

Updated

10 years ago
Attachment #391703 - Attachment description: add libs to _DEPS and set up vpaths → add libs to _DEPS and set up vpaths v2
Attachment #391703 - Flags: review?(benjamin)

Comment 11

10 years ago
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.
Attachment #391703 - Flags: review?(benjamin) → review+
Assignee

Comment 12

10 years ago
http://hg.mozilla.org/mozilla-central/rev/8fa93262b47b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3
Target Milestone: mozilla1.9.3 → mozilla1.9.3a1

Updated

10 years ago
Depends on: 511945
Depends on: 514393
Flags: in-testsuite-
Attachment #428607 - Flags: review?(bugspam.Callek) → review+
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
Attachment #428607 - Attachment description: (Bv1-CC) Copy it to comm-central → (Bv1-CC) Copy it to comm-central [Checkin: Comment 14]
Attachment #391703 - Attachment description: add libs to _DEPS and set up vpaths v2 → add libs to _DEPS and set up vpaths v2 [Checkin: Comment 12]

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.