751167 - makefiles: $(MAKE) -C mobile/.../locales libs call should be a nop for dependency builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

gmake -C ../locales libs
gmake[2]: Entering directory `/local/mozilla/bugs/748470/objdir-droid/mobile/locales'
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/JarMaker.py \
   -j ../../dist/bin/chrome \
  -t /local/mozilla/bugs/748470 -f flat  -c /local/mozilla/bugs/748470/mobile/locales/en-US -DNDEBUG -DTRIMMED -DAB_CD=en-US  -DANDROID=1 -DANDROID_VERSION=5 -DCROSS_COMPILE=1 -DX_DISPLAY_MISSING=1 -DMOZ_THUMB2=1 -DHAVE_ARM_SIMD=1 -DHAVE_ARM_NEON=1 -DMOZ_ENABLE_PROFILER_SPS=1 -DMOZILLA_VERSION=\"15.0a1\" -DMOZILLA_VERSION_U=15.0a1 -DMOZILLA_UAVERSION=\"15.0\" -DNO_PW_GECOS=1 -DMOZ_LINKER=1 -DD_INO=d_ino -DSTDC_HEADERS=1 -DHAVE_SSIZE_T=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DHAVE_UINT=1 -DHAVE_UINT_T=1 -DHAVE_UNAME_DOMAINNAME_FIELD=1 -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE=1 -DHAVE_VISIBILITY_ATTRIBUTE=1 -DHAVE_DIRENT_H=1 -DHAVE_GETOPT_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_MALLOC_H=1 -DHAVE_SYS_STATFS_H=1 -DHAVE_SYS_SYSMACROS_H=1 -DHAVE_LINUX_QUOTA_H=1 -DHAVE_MMINTRIN_H=1 -DHAVE_SYS_CDEFS_H=1 -DHAVE_DLOPEN=1 -DHAVE_MEMMEM=1 -DNO_X11=1 -DHAVE_STRERROR=1 -DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1 -DHAVE_SNPRINTF=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DHAVE_SETBUF=1 -DHAVE_ISATTY=1 -DHAVE_FLOCKFILE=1 -DHAVE_LOCALTIME_R=1 -DHAVE_STRTOK_R=1 -DHAVE_CLOCK_MONOTONIC=1 -DMALLOC_H=\<malloc.h\> -DHAVE_STRNDUP=1 -DHAVE_MEMALIGN=1 -DHAVE_VALLOC=1 -DHAVE_I18N_LC_MESSAGES=1 -DNS_ALWAYS_INLINE=__attribute__\(\(always_inline\)\) -DNS_ATTR_MALLOC=__attribute__\(\(malloc\)\) -DNS_WARN_UNUSED_RESULT=__attribute__\(\(warn_unused_result\)\) -DMOZ_BUILD_APP=mobile/android -DMOZ_WIDGET_ANDROID=1 -DMOZ_PDF_PRINTING=1 -DMOZ_INSTRUMENT_EVENT_LOOP=1 -DUSE_ARM_KUSER=1 -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DMOZ_ANDROID_HISTORY=1 -DMOZ_JAVA_COMPOSITOR=1 -DIBMBIDI=1 -DACCESSIBILITY=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_RAW=1 -DMOZ_OGG=1 -DATTRIBUTE_ALIGNED_MAX=64 -DMOZ_WEBM=1 -DVPX_ARM_ASM=1 -DMOZ_WAVE=1 -DMOZ_SYDNEYAUDIO=1 -DMOZ_MEDIA=1 -DMOZ_TREMOR=1 -DMOZ_XTF=1 -DENABLE_SYSTEM_EXTENSION_DIRS=1 -DMOZ_CRASHREPORTER=1 -DMOZ_CRASHREPORTER_ENABLE_PERCENT=100 -DLIBJPEG_TURBO_ARM_ASM=1 -DMOZ_USE_NATIVE_POPUP_WINDOWS=1 -DMOZ_TREE_FREETYPE=1 -DHAVE_FT_BITMAP_SIZE_Y_PPEM=1 -DHAVE_FT_GLYPHSLOT_EMBOLDEN=1 -DHAVE_FT_LOAD_SFNT_TABLE=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_DISABLE_DOMCRYPTO=1 -DMOZ_FEEDS=1 -DMOZ_GFX_OPTIMIZE_MOBILE=1 -DMOZ_DEBUG_SYMBOLS=1 -DMOZ_LOGGING=1 -DSIZEOF_INT_P=4 -DMOZ_MEMORY_SIZEOF_PTR_2POW=2 -DMOZ_MEMORY=1 -DMOZ_MEMORY_LINUX=1 -DMOZ_MEMORY_ANDROID=1 -DJSGC_INCREMENTAL=1 -DHAVE__UNWIND_BACKTRACE=1 -DJS_DEFAULT_JITREPORT_GRANULARITY=3 -DMOZ_OMNIJAR=1 -DMOZ_USER_DIR=\".mozilla\" -DMOZ_STATIC_JS=1 -DHAVE_STDINT_H=1 -DHAVE_INTTYPES_H=1 -DMOZ_TREE_CAIRO=1 -DHAVE_UINT64_T=1 -DMOZ_TREE_PIXMAN=1 -DMOZ_GRAPHITE=1 -DMOZ_ENABLE_SKIA=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DBUILD_CTYPES=1 -DMOZ_PLACES=1 -DMOZ_APP_COMPONENT_INCLUDE=\"nsBrowserComponents.h\" -DMOZ_APP_UA_NAME=\"Firefox\" -DMOZ_APP_UA_VERSION=\"15.0a1\" -DMOZ_UA_FIREFOX_VERSION=\"15.0a1\" -DFIREFOX_VERSION=15.0a1 -DMOZ_DLL_SUFFIX=\".so\" -DXP_UNIX=1 -DUNIX_ASYNC_DNS=1  \
  /local/mozilla/bugs/748470/mobile/locales/jar.mn
processing /local/mozilla/bugs/748470/mobile/locales/jar.mn
gmake[2]: Leaving directory `/local/mozilla/bugs/748470/objdir-droid/mobile/locales'


 * Also beneath android/chrome

/objdir-droid/mobile/android/chrome'
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/JarMaker.py \
   -j ../../../dist/bin/chrome \
  -t /local/mozilla/bugs/748470 -f flat  -DNDEBUG -DTRIMMED -DAB_CD=en-US -DPACKAGE=browser -DMOZ_APP_VERSION=15


 * objdir-droid/mobile/android/modules  ( enough for now, individual bugs will be needed )

gmake[2]: Entering directory `/local/mozilla/bugs/748470/objdir-droid/mobile/android/modules'
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -R -m 644 /local/mozilla/bugs/748470/mobile/android/modules/LocaleRepository.jsm /local/mozilla/bugs/748470/mobile/android/modules/linuxTypes.jsm /local/mozilla/bugs/748470/mobile/android/modules/video.jsm ../../../dist/bin/modules
set -e;  \
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../dist/bin/modules; \
for i in /local/mozilla/bugs/748470/mobile/android/modules/contacts.jsm; do \
  dest=../../../dist/bin/modules/`basename $i`; \
  rm -f -f $dest; \
  /usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py  -DANDROID=1 -DANDROID_VERSION=5 -DCROSS_COMPILE=1 -DX_DISPLAY_MISSING=1 -DMOZ_THUMB2=1 -DHAVE_ARM_SIMD=1 -DHAVE_ARM_NEON=1 -DMOZ_ENABLE_PROFILER_SPS=1 -DMOZILLA_VERSION=\"15.0a1\" -DMOZILLA_VERSION_U=15.0a1 -DMOZILLA_UAVERSION=\"15.0\" -DNO_PW_GECOS=1 -DMOZ_LINKER=1 -DD_INO=d_ino -DSTDC_HEADERS=1 -DHAVE_SSIZE_T=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DHAVE_UINT=1 -DHAVE_UINT_T=1 -DHAVE_UNAME_DOMAINNAME_FIELD=1 -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE=1 -DHAVE_VISIBILITY_ATTRIBUTE=1 -DHAV

Comment 1

[Joey Armstrong [:joey]]

Attachment: Replace FORCEd work with makefile dependencies.

- Seriously considering splitting this patch into a few smaller ones.  At a minimum 2 for independent dep generation of bookmarks.json and the searchlist.

1) Split target rules for bookmarks and searchlist into -preqs, target and timestamp.
2) Move items of contention and state (targets, deps, etc) into a locale specific directory.
3) -preqs are used to regenerate targets based on dependency.
4) An explicit FORCE for language repacks is also listed.
5) Top level/common targets will hard link common name to the current locale specific elements.  This logic is essentially FORCE for repacks built by dependency.
6) Added $(GLOBAL_DEPS) to force rebuilding when makefiles are modified.
7) Replaced directory dependencies with calls to mkdir_deps.

Notes inserted around problem questions, questionable behavior and content for new bug sources.

Comment 2

[Joey Armstrong [:joey]]

Ignore plugin_file -vs- plugin-file, syntax to be made consistent.

Comment 3

[Joey Armstrong [:joey]]

>> MOZ_BRANDING_DIRECTORY should never be empty in a Mozilla build. I thought there was fallback logic in configure to make sure it's always defined, but it looks like maybe that's not the case (there's fallback for REAL_BRANDING_DIRECTORY, but it doesn't actually assign to MOZ_BRANDING_DIRECTORY). Erroring out when it isn't defined is fine.
>>
>> Gavin

Can either report the error locally with $(call errorIfEmpty,MOZ_BRANDING_DIRECTORY) or modify config.mk to morph variable dereferences into an error:
MOZ_BRANDING_DIRECTORY ?= $(error var is not set)

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 623124 [details] [diff] [review]
Replace FORCEd work with makefile dependencies.

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

A few comments, this mostly looks good. The only thing I'm concerned about is the portability of your ln commands.

::: config/rules.mk
@@ +1612,5 @@
>  	$(LOOP_OVER_PARALLEL_DIRS)
>  	$(LOOP_OVER_DIRS)
>  	$(LOOP_OVER_TOOL_DIRS)
>  
> +$(FINAL_TARGET)/chrome: $(call mkdir_deps,$(FINAL_TARGET)/chrome)

I don't think this is what you want, is it? You instead want to replace $(FINAL_TARGET)/chrome in the deps below with the mkdir_deps. This way you still have a goofy directory dependency in the list.

::: mobile/locales/Makefile.in
@@ +77,5 @@
> +#   gmake -C $obj/mobile/locales
> +# $(wildcard $(MOZILLA_DIR) $(topsrcdir) could be used to find an available
> +# command but cannot think of a good reason to use a variant version of
> +# JarMaker.py when MOZILLA_DIR!=$(topsrcdir) or MOZILLA_DIR is over-ridden.
> +###########################################################################

This doesn't really make sense to me given this code:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#13

I'm pretty sure the only reason for MOZILLA_DIR is for Thunderbird/Seamonkey builds where they have mozilla-central checked out inside a comm-central repo, so for Fennec it probably doesn't matter either way.

@@ +87,5 @@
> +# ?-COMMENT TO BE REMOVED
> +# todo: unsure if vpath is still needed.  Likely
> +# source would be bookmarks-$(AB_CD).inc passed in
> +# by an external source but no local deps for this.
> +####################################################

I wouldn't fiddle with any of this if you can help it, unless you want to rewrite it from scratch.

@@ +102,5 @@
> +#
> +# Is interaction with Makefile at the top level OK when MOZ_BRANDING*
> +# is undefined or should this case be handled as an error or condtional ?
> +###########################################################################
> +$(call warnIfEmpty,MOZ_BRANDING_DIRECTORY)

As bsmedberg said in the other bug you filed on this, for Firefox/Fennec, this should always be defined, so you should be able to make this an error in this particular makefile.

@@ +118,5 @@
> +# ?-Comment to be removed
> +# Default target, preserve existing functionality for:
> +#    gmake -C $obj/mobile/locales
> +###########################################################################
> +search-jar-default: search-jar

This is kind of weird, in general we want the default targets in rules.mk to be the defaults. This might be a bug in this Makefile. It's probably not worth changing the behavior in this patch though.

@@ +145,5 @@
> +plugin-file-ts = $(tgt-gendir)/$(subst $(topsrcdir)/,$(NULL),$(plugin_file)).ts
> +
> +GARBAGE += $(plugin-file-ts)
> +# ---------------------------------------------------------------------------
> +# plugin-file-ts track searchlist file used ($path/list.txt) and TLM

TLM?

@@ +175,5 @@
> +# ---------------------------------------------------------------------------
> +# TODO: -common
> +#   jarfile has been replaced by a symlink pointing at a locale specific
> +#   file generated by dependency.  Symlink can be removed when no external
> +#   consumers are dependent on the filename.

I don't *think* anything depends on this filename, does it? A quick MXR search doesn't show anything:
http://mxr.mozilla.org/mozilla-central/search?string=tmp-search.jar.mn

AFAICT it's simply a placeholder to stick this info to pass to JarMaker.py.

@@ +193,2 @@
>  	printf "$(AB_CD).jar:" > $@
> +	ln --logical -fn $@ .

I would guess that --logical isn't portable. Keep in mind that we have lots of devs building Fennec on OS X, which doesn't use GNU tools.

@@ +197,5 @@
>  
> +.PHONY: searchplugins
> +searchplugins: $(search-jar-ts)
> +	@echo "\nGenerating: $@"	
> +	ln --logical -fn $(search-jar) .

Same thing here.

@@ +214,5 @@
> +# Is this detecting bulk content changes or a dep for mkdir ?
> +# Target files are generated into the chrome directory so this dependency
> +# essentially a stealth FORCE.  A wildcard + filtering could remove the
> +# explicit FORCE behavior.  Replacing the dep with mkdir_dep would be
> +# even easier.

This is a dep for mkdir. We just have a lot of poorly-written makefiles. Just fix it!

@@ +284,5 @@
> +bookmarks-inc-array = \
> +  $(wildcard \
> +    $(if $(has_mergedir),$(LOCALE_MERGEDIR)/mobile/profile/bookmarks.inc) \
> +    $(LOCALE_SRCDIR)/profile/bookmarks.inc \
> +    $(if $(has-mergedir),@srcdir@/en-US/profile/bookmarks.inc) \

Not sure why this uses @srcdir@ but you should be able to safely replace that with $(srcdir). (@srcdir@ gets substed by configure).

Comment 5

[Joey Armstrong [:joey]]

(In reply to Ted Mielczarek [:ted] from comment #4)
> Comment on attachment 623124 [details] [diff] [review]
> Replace FORCEd work with makefile dependencies.
> 
> Review of attachment 623124 [details] [diff] [review]:
> -----------------------------------------------------------------

::: config/rules.mk
@@ +1612,5 @@
> +$(FINAL_TARGET)/chrome: $(call mkdir_deps,$(FINAL_TARGET)/chrome)
>>
>> I don't think this is what you want, is it? You instead want to replace $(FINAL_TARGET)/chrome in the deps below with the mkdir_deps. This way you still have a goofy directory dependency in the list.

The dep was added to prevent multiple mkdir calls from being made { that and fix a bootstrap problem when nsinstall has yet to be built ).  Yes deps within Makefile.in should be changed explicitly to mkdir_deps -- since the dep is not involved with detecting bulk changes.


> ---------------------------------------------------------------------------
> > +# plugin-file-ts track searchlist file used ($path/list.txt) and TLM
> 
> TLM?

timestamp/time last modified, I'll expand the text


> @@ +175,5 @@
> > +# ---------------------------------------------------------------------------
> > +# TODO: -common
> > +#   jarfile has been replaced by a symlink pointing at a locale specific
> > +#   file generated by dependency.  Symlink can be removed when no external
> > +#   consumers are dependent on the filename.
> 
> I don't *think* anything depends on this filename, does it? A quick MXR
> search doesn't show anything:
> http://mxr.mozilla.org/mozilla-central/search?string=tmp-search.jar.mn

Probably not, just trying to limit potential for fallout by not removing files on disk.
 
> AFAICT it's simply a placeholder to stick this info to pass to JarMaker.py.

Clobber builds do work fine w/o the file but multi-locales have yet to be checked.

Comment 6

[Joey Armstrong [:joey]]

Attachment: replace FORCE with makefile dependencies

Comment 7

[Joey Armstrong [:joey]]

Comment on attachment 623720 [details] [diff] [review]
replace FORCE with makefile dependencies

>> I don't think this is what you want, is it?

$(FINAL_TARGET)/chrome dep replaced with mkdir_dep in mobile/locales/Makefile.in to remove stealth FORCE condition.  Target rule persists in rules.mk in case other deps still rely on the path target.

Added a call to errorIfEmpty(MOZ_BRANDING_DIR).

Changed hardlink creation to softlinks for bookmarks and searchjar to avoid command portability problems.

Heh also replaced @srcdir@ reference with $(srcdir), did not notice that hardcoded path expansion.

Results from a try job are on the way.

Comment 8

[Joey Armstrong [:joey]]

Attachment: replace FORCE with makefile dependencies

Comment 9

[Joey Armstrong [:joey]]

Comment on attachment 623748 [details] [diff] [review]
replace FORCE with makefile dependencies

Same patch as before.
Removed stray debugging line in search-jar target.
Re-add search for $(MOZILLA_DIR)/config/JarMaker.py, try fails trying to use the MOZILLA_DIR command path.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 623748 [details] [diff] [review]
replace FORCE with makefile dependencies

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

Just a few nits, looks good otherwise.

::: config/rules.mk
@@ +1604,5 @@
>  	$(LOOP_OVER_PARALLEL_DIRS)
>  	$(LOOP_OVER_DIRS)
>  	$(LOOP_OVER_TOOL_DIRS)
>  
> +$(FINAL_TARGET)/chrome: $(call mkdir_deps,$(FINAL_TARGET)/chrome)

Can you file a followup on removing this? I doubt it's used by anything outside of rules.mk, but I understand being cautious.

::: mobile/locales/Makefile.in
@@ +130,5 @@
> +###########################################################################
> +# Detect locale changes.  Force stale deps when searchlist file
> +# or content has changed.
> +$(plugin-file-ts): $(plugin-file-ts-preqs)
> +	@touch $@

You use $(TOUCH) elsewhere, should be consistent.

@@ +143,5 @@
> +# ---------------------------------------------------------------------------
> +# search-jar contains a list of providers for the search plugin
> +###########################################################################
> +SEARCH_PLUGINS = $(shell cat $(plugin_file))
> +$(call errorIfEmpty,SEARCH_PLUGINS)

Is this going to wind up invoking the cat more than once, since the assignment is = and not :=?

@@ +158,2 @@
>  	printf "$(AB_CD).jar:" > $@
> +	ln -fn $@ .

Is there any reason you stuck this in the middle of generating the file, and not at the end?

@@ +196,5 @@
> +  $(call mkdir-deps,$(DIST)/install) \
> +  $(NULL)
> +
> +libs-%: $(libs-preqs)
> +	$(display-deps)

Is this leftover debug code?

@@ +250,5 @@
> +.PHONY: bookmarks $(bookmarks)
> +bookmarks: $(bookmarks)
> +$(bookmarks): $(bookmarks-ts)
> +	@echo "\nGenerating: $@"
> +	ln -fn $< .

This is sort of odd because you call $(bookmarks) .PHONY, but the ln here does actually create the file.

Comment 11

[Joey Armstrong [:joey]]

(In reply to Ted Mielczarek [:ted] from comment #10)
> Comment on attachment 623748 [details] [diff] [review]
> replace FORCE with makefile dependencies
> 
> Review of attachment 623748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a few nits, looks good otherwise.
> 
> ::: config/rules.mk
> @@ +1604,5 @@
> >  	$(LOOP_OVER_PARALLEL_DIRS)
> >  	$(LOOP_OVER_DIRS)
> >  	$(LOOP_OVER_TOOL_DIRS)
> >  
> > +$(FINAL_TARGET)/chrome: $(call mkdir_deps,$(FINAL_TARGET)/chrome)
> 
> Can you file a followup on removing this? I doubt it's used by anything
> outside of rules.mk, but I understand being cautious.

New bug opened

 
> @@ +143,5 @@
> > +# ---------------------------------------------------------------------------
> > +# search-jar contains a list of providers for the search plugin
> > +###########################################################################
> > +SEARCH_PLUGINS = $(shell cat $(plugin_file))
> > +$(call errorIfEmpty,SEARCH_PLUGINS)
> 
> Is this going to wind up invoking the cat more than once, since the
> assignment is = and not :=?

The value will need to change if different list.txt files are loaded by repacks so the value cannot be cached by := here.  Also deps will limit when the command is run, when the searchlist contents are updated.

 
> @@ +158,2 @@
> >  	printf "$(AB_CD).jar:" > $@
> > +	ln -fn $@ .
> 
> Is there any reason you stuck this in the middle of generating the file, and
> not at the end?

Yes, if the target were to fail early the symlink file may not reference the correct file.  Confusing for debugging because contents of the last file referenced would appear to be valid.

Comment 12

[Joey Armstrong [:joey]]

https://tbpl.mozilla.org/?tree=Try&rev=d7bb330d5579

Try job passed, submitting to inbound

Comment 13

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Joey Armstrong [:joey] from comment #12)
> https://tbpl.mozilla.org/?tree=Try&rev=d7bb330d5579
> 
> Try job passed, submitting to inbound

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f7c93830e858

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f7c93830e858