Closed Bug 771287 Opened 9 years ago Closed 9 years ago

makefiles: unit test failure from xpidl.mk edits

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: joey, Assigned: joey)

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch xpidl unit test failure (obsolete) — Splinter Review
Comment on attachment 639444 [details] [diff] [review]
xpidl unit test failure

simple edit to make the problem more visible.
Attached patch xpidl unit test failure (obsolete) — Splinter Review
Attachment #639444 - Attachment is obsolete: true
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.
Attachment #639451 - Flags: review?(khuey)
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)
(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 on attachment 639451 [details] [diff] [review]
xpidl unit test failure

Ted's already looked at this, so I'll let him finish.
Attachment #639451 - Flags: review?(khuey) → review?(ted.mielczarek)
Attachment #639451 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → 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.
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.
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.
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.
Attachment #639451 - Attachment is obsolete: true
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.
inbound: commited changeset 101693:fa134d4bb470
https://hg.mozilla.org/mozilla-central/rev/fa134d4bb470
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.