config/rules.mk: use of $(dir) by _INSTALL_TESTS and libs::xpcshell-tests

NEW
Unassigned

Status

Firefox Build System
General
6 years ago
3 months ago

People

(Reporter: joey, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

6 years ago
Not yet sure if this is a problem but at a minimum variable names should be changed in the makefile for clarity.


config/rules.mk defines a canned sequence to gather files for xpcshell tests:
=============================================================================
define _INSTALL_TESTS
$(DIR_INSTALL) $(wildcard $(srcdir)/$(dir)/*) $(testxpcobjdir)/$(relativesrcdir)/$(dir)

endef # do not remove the blank line!


The libs:: target setup to make use of $(_INSTALL_TESTS) is also using a variable $(dir) in a make $(foreach dir) loop will iterate over paths to check:

libs::
	$(foreach dir,$(XPCSHELL_TESTS),$(_INSTALL_TESTS))
ifndef NO_XPCSHELL_MANIFEST_CHECK
	$(PYTHON) $(MOZILLA_DIR)/build/xpccheck.py \
	  $(topsrcdir) \
	  $(topsrcdir)/testing/xpcshell/xpcshell.ini \
	  $(addprefix $(MOZILLA_DIR)/$(relativesrcdir)/,$(XPCSHELL_TESTS))
endif


_INSTALL_TESTS $(dir) var will expand before lib's $(foreach dir) so the foreach loop will iterate over files collected from a parent directory rather than the $(dir) subdirectory of parent.

It would be possible to assign a value to dir= prior to $(_INSTALL_TESTS) expansion of $(dir) to gather from subdirs .. independent of the $(foreach dir) iteration but overloaded use of $(dir) is becomes cryptic.

chrome/test/Makefile gathered paths of: chrome/test//unit 


One of the following should correct usage for _INSTALL_TESTS::dir
  o test $(dir) and exit with error if a value of dir is required but empty.
  o use distinct variable names to distinguish the library function and caller usage.
  o if $(dir) use by _INSTALL_TESTS is conditional for parent -vs- subdir file collection.  Path construction can be made conditional by $(if $(dir),$(dir)/) to exclude stray directory slashes.



DIR_INSTALL:   /local/mozilla/bugs/687515/obj-x86_64-unknown-linux-gnu/config/nsinstall -R
  srcdir=[/local/mozilla/bugs/687515/chrome/test]
  dir=[]
  testxpcobjdir=[../../_tests/xpcshell]
  relativesrcdir=[chrome/test]

INSTALL_TESTS:  /local/mozilla/bugs/687515/obj-x86_64-unknown-linux-gnu/config/nsinstall -R /local/mozilla/bugs/687515/chrome/test//unit_ipc /local/mozilla/bugs/687515/chrome/test//Makefile.in /local/mozilla/bugs/687515/chrome/test//unit ../../_tests/xpcshell/chrome/test/
(Reporter)

Comment 1

6 years ago
After a fresh look at the syntax of dir in $(foreach dir,,$(_INSTALL_TESTS)) should be OK.  $(_INSTALL_TESTS) would be the target of $(foreach dir) and assigned subdir values with each loop iteration.  Still unclear why the value of $(dir) was $(NULL) within $(_INSTALL_TESTS) but using a canned sequence in this way is valid.

Converting _INSTALL_TESTS into a user function invoked by $(call func,$(dir)) would help make the argument handling clear and may resolve the $(dir) usage problem.

Updated

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