771287 - makefiles: unit test failure from xpidl.mk edits

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

cd obj-x86_64-unknown-linux-gnu/config/makefiles/test
gmake check

gmake -f /local/mozilla/bugs/foo/config/makefiles/test/check-xpidl.mk check-xpidl 
mkdir -p blah/idl/
mkdir -p blah/dist/include/
gmake[1]: *** [check-xpidl] Error 91

error 91 detected a failure while installing to dist.
Problem was caused by $(call install_cmd), config/config.mk is not included as it would create far too many permutations for testing.  Unclear how 98137:88aaf6c529b9 might land with this failure mode present.

Unit testing will need to declare install_cmd locally to avoid a dependency on config.mk

Comment 1

[Joey Armstrong [:joey]]

Attachment: xpidl unit test failure

Comment 2

[Joey Armstrong [:joey]]

Comment on attachment 639444 [details] [diff] [review]
xpidl unit test failure

simple edit to make the problem more visible.

Comment 3

[Joey Armstrong [:joey]]

Attachment: xpidl unit test failure

Comment 4

[Joey Armstrong [:joey]]

Comment on attachment 639451 [details] [diff] [review]
xpidl unit test failure

Added a $(requiredfunction) check for install_cmd so problems will be more visible.

Unit test failure was due to install_cmd not being defined.  Including config.mk would be expensive for testing, adding a large amount of logic and permutations to handle.

For now keep test logic simple and avoid un-necessary deps by inlining a local definition for the function.

Comment 5

[Joey Armstrong [:joey]]

Question: shouldn't the install command be able to handle more than one file at a time ?

[phantasm:771287] grep install_cmd config/config.mk

# Single-line commands should be switched over to install_cmd.
install_cmd = $(NSINSTALL_NATIVECMD) -t $(1)

# The default for install_cmd is simply INSTALL
install_cmd ?= $(INSTALL) $(1)

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Joey Armstrong [:joey] from comment #5)
> Question: shouldn't the install command be able to handle more than one file
> at a time ?

All the files to install are passed (space separated) in $(1), AIUI.

Also, do these unit tests not get run as part of "make check" on tinderbox builds?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 639451 [details] [diff] [review]
xpidl unit test failure

Ted's already looked at this, so I'll let him finish.

Comment 8

[Joey Armstrong [:joey]]

(In reply to Ted Mielczarek [:ted] from comment #6)
>
> Also, do these unit tests not get run as part of "make check" on tinderbox
> builds?

Yes they are run by dependency when xpidl.mk, etc are modified.  If the test did not run/fail in this case it is probably due to the redefinition of 'INSTALL'.

This test does not place deps on rules.mk or config.mk as that would force having to run the test unnecessarily with every edit.

Comment 9

[(not currently active) Ted Mielczarek]

I think we should change that behavior. Conventionally we run all our tests unconditionally. So all of our build system tests run as part of "make check", regardless of whether something has changed. This prevents a situation like this from happening, where changes to other parts of the system break functionality or a test elsewhere.

Comment 10

[Joey Armstrong [:joey]]

committed changeset 100436:fda288d0724d

ted>
I'll have to send another patch through to add the extra test deps.
Maybe the install* related commands could be isolated into a smaller dependency { kinda doubt it for this specifically } so the test will only run when the makefile or install command changes.

These unit tests are small but running unconditionally will add to job times as they accumulate.

Comment 11

[Ed Morley [:emorley]]

Backed out for check-sync-dirs failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/061db7f824e4

Comment 12

[(not currently active) Ted Mielczarek]

I really think that running all the tests on every commit is the only sane behavior. Yes, it gets slower as we add more tests, but this is why we have tests.

Comment 13

[Joey Armstrong [:joey]]

Attachment: xpidl unit test failure

Comment 14

[Joey Armstrong [:joey]]

r=ted carried forward.
dup xpidl.mk edits to make check-sync-dirs happy.
add config/config.mk as a dependency for the check-xpidl.mk unit test and removed explicit install_cmd= def to add test coverage.

try job running.

Comment 15

[Joey Armstrong [:joey]]

inbound: commited changeset 101693:fa134d4bb470

Comment 16

[Ed Morley [:emorley]]

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