Last Comment Bug 771287 - makefiles: unit test failure from xpidl.mk edits
: makefiles: unit test failure from xpidl.mk edits
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-05 13:05 PDT by Joey Armstrong [:joey]
Modified: 2012-08-08 09:34 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
xpidl unit test failure (914 bytes, patch)
2012-07-05 13:09 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Review
xpidl unit test failure (1.72 KB, patch)
2012-07-05 13:37 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Review
xpidl unit test failure (3.44 KB, patch)
2012-07-25 12:30 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Review

Description Joey Armstrong [:joey] 2012-07-05 13:05:33 PDT
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] 2012-07-05 13:09:21 PDT
Created attachment 639444 [details] [diff] [review]
xpidl unit test failure
Comment 2 Joey Armstrong [:joey] 2012-07-05 13:10:23 PDT
Comment on attachment 639444 [details] [diff] [review]
xpidl unit test failure

simple edit to make the problem more visible.
Comment 3 Joey Armstrong [:joey] 2012-07-05 13:37:50 PDT
Created attachment 639451 [details] [diff] [review]
xpidl unit test failure
Comment 4 Joey Armstrong [:joey] 2012-07-05 13:41:10 PDT
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] 2012-07-05 13:42:58 PDT
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 Ted Mielczarek [:ted.mielczarek] 2012-07-09 11:06:31 PDT
(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 [:khuey] (khuey@mozilla.com) 2012-07-16 08:45:22 PDT
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] 2012-07-25 06:43:10 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2012-07-25 06:48:09 PDT
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] 2012-07-25 09:50:06 PDT
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] 2012-07-25 10:11:06 PDT
Backed out for check-sync-dirs failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/061db7f824e4
Comment 12 Ted Mielczarek [:ted.mielczarek] 2012-07-25 12:09:04 PDT
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] 2012-07-25 12:30:54 PDT
Created attachment 645856 [details] [diff] [review]
xpidl unit test failure
Comment 14 Joey Armstrong [:joey] 2012-07-25 12:33:38 PDT
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] 2012-08-07 11:43:59 PDT
inbound: commited changeset 101693:fa134d4bb470
Comment 16 Ed Morley [:emorley] 2012-08-08 09:34:57 PDT
https://hg.mozilla.org/mozilla-central/rev/fa134d4bb470

Note You need to log in before you can comment on or make changes to this bug.