Closed
Bug 488091
Opened 16 years ago
Closed 8 years ago
Review |make check| usages
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
mozilla1.9.2a1
People
(Reporter: sgautherie, Assigned: sgautherie)
References
Details
(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1b99: Av2a])
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•16 years ago
|
||
Passed on Tryserver as 'try-9534e0d4ded'.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #372585 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•16 years ago
|
Attachment #372587 -
Flags: review? → review?(philipp)
Comment 3•16 years ago
|
||
Comment on attachment 372587 [details] [diff] [review] (Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS [Checkin: See comment 5] r=philipp
Attachment #372587 -
Flags: review?(philipp) → review+
Updated•16 years ago
|
Attachment #372585 -
Flags: review?(ted.mielczarek) → review-
Comment 4•16 years ago
|
||
Comment on attachment 372585 [details] [diff] [review] (Av1) Remove empty targets, add ifdef, use PARRALEL_DIRS What's the point of using PARALLEL_DIRS here? It doesn't give any advantage unless you have more than one directory in it.
Assignee | ||
Comment 5•16 years ago
|
||
Comment on attachment 372587 [details] [diff] [review] (Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS [Checkin: See comment 5] http://hg.mozilla.org/comm-central/rev/c8cdbc3a283c Bv1-CC, target removal only.
Attachment #372587 -
Attachment description: (Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS → (Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS
[Checkin: See comment 5]
Assignee | ||
Comment 6•16 years ago
|
||
Av1, with comment 4 suggestion(s).
Attachment #372585 -
Attachment is obsolete: true
Attachment #374837 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 7•16 years ago
|
||
I tried this patch on Try server and the tests are not executed anymore. Then I wonder about http://hg.mozilla.org/mozilla-central/diff/70dd0e9f14c2/security/manager/Makefile.in (This seems to be the only place where ENABLE_TESTS is used in nsprpub+nss+security...) Honza, do your ENABLE_TESTS actually work? What would I be doing wrong?
Attachment #374911 -
Flags: review?(wtc)
Attachment #374911 -
Flags: review?(honzab.moz)
Comment 8•16 years ago
|
||
Comment on attachment 374911 [details] [diff] [review] (Cv1) </security/>: Remove empty targets, add ifdef Serge, thanks for the patch. Your change to security/manager/ssl/Makefile.in is good. I believe that your changes to security/manager/ssl/tests/Makefile.in and security/manager/ssl/tests/mochitest/Makefile.in are not necessary because we only recurse into security/manager/ssl/tests if ENABLE_TESTS is defined, due to your change to security/manager/ssl/Makefile.in. I would not modify security/nss/tests/pkcs11/netscape/trivial/Makefile.in because it is obsolete code. If you really want to remove the no-op targets, please also update this comment: > # Now, various standard targets, some that do stuff, some that are no-ops
Attachment #374911 -
Flags: review?(wtc) → review-
Comment 9•16 years ago
|
||
Comment on attachment 374837 [details] [diff] [review] (Av2) Remove empty targets, add ifdef [Checkin: See comment 10 & 11] --- a/modules/libpr0n/test/Makefile.in +ifdef ENABLE_TESTS DIRS += mochitest +endif Leave this out, this test directory is already ifdef ENABLE_TESTS in the parent Makefile.
Attachment #374837 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 10•16 years ago
|
||
Comment on attachment 374837 [details] [diff] [review] (Av2) Remove empty targets, add ifdef [Checkin: See comment 10 & 11] http://hg.mozilla.org/mozilla-central/rev/20ab96ddb645 Av2, with comment 9 suggestion(s).
Attachment #374837 -
Attachment description: (Av2) Remove empty targets, add ifdef → (Av2) Remove empty targets, add ifdef
[Checkin: See comment 10]
Assignee | ||
Comment 11•16 years ago
|
||
Comment on attachment 374837 [details] [diff] [review] (Av2) Remove empty targets, add ifdef [Checkin: See comment 10 & 11] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/90222479919c
Attachment #374837 -
Attachment description: (Av2) Remove empty targets, add ifdef
[Checkin: See comment 10] → (Av2) Remove empty targets, add ifdef
[Checkin: See comment 10 & 11]
Assignee | ||
Updated•16 years ago
|
Updated•16 years ago
|
Attachment #374911 -
Flags: review?(honzab.moz)
Comment 12•16 years ago
|
||
Comment on attachment 374911 [details] [diff] [review] (Cv1) </security/>: Remove empty targets, add ifdef If I understand correctly what the patch has to do I can say I have same objections as Wan-Teh in comment 8. (In reply to comment #7) > Then I wonder about > http://hg.mozilla.org/mozilla-central/diff/70dd0e9f14c2/security/manager/Makefile.in > (This seems to be the only place where ENABLE_TESTS is used in > nsprpub+nss+security...) > > Honza, do your ENABLE_TESTS actually work? > What would I be doing wrong? This hunk compiles some binaries (utils to manipulate certificates) needed to have SSL support in mochitest. It works, AFAIK. I don't know what has been going wrong on the tryserver, could you refer the log or did you try the patch locally? If you wish I can do it my self if you more describe what is wrong in detail. I'm not that familiar with what you are trying to achieve: 'make check' have to invoke all tests (xpc, ref, mochi, chrome, browser...) and therefor you made a new target to run xpc-tests?
Assignee | ||
Comment 13•16 years ago
|
||
Cv1, with comment 8 suggestion(s), and some more. I could remove the 'check' shortcut if you prefer, otherwise here is the new/fixed 'xpcshell-tests' shortcut. |+include $(DEPTH)/config/autoconf.mk| That's why ENABLE_TESTS was not defined in that file! I guess it's all right to include it here? Do you want to include it in the ('tests') sub-directories too, just to be future-proof? (In reply to comment #8) > I would not modify security/nss/tests/pkcs11/netscape/trivial/Makefile.in > because it is obsolete code. It does looks like so: would it be possible to rather remove some of these files? (In reply to comment #12) > This hunk compiles some binaries (utils to manipulate certificates) needed to > have SSL support in mochitest. It works, AFAIK. Indeed, I verified that :-) > I don't know what has been going wrong on the tryserver I eventually found out, thanks for having looked at it, Honza.
Attachment #374911 -
Attachment is obsolete: true
Attachment #375630 -
Flags: review?(wtc)
Comment 14•16 years ago
|
||
Comment on attachment 375630 [details] [diff] [review] (Cv2) </security/>: update shortcut, add ifdef, remove empty targets r=wtc. 1. I don't understand your change to security/manager/Makefile.in. 2. In security/manager/ssl/Makefile.in, I would try to align the = and += with the = signs above. 3. Please exclude security/nss/tests/pkcs11/netscape/trivial/Makefile.in from this patch. security/nss/tests/pkcs11 is obsolete code that hope will be resurrected some day. Until then let's not fix cosmetic problems in it because we can't test the patch.
Attachment #375630 -
Flags: review?(wtc) → review+
Assignee | ||
Comment 15•16 years ago
|
||
(In reply to comment #14) > 1. I don't understand your change to security/manager/Makefile.in. Bug 485736 split 'check' target into 'xpcshell-tests'; 'ssl' directory doesn't seem to execute anything, the tests are in 'ssl/tests' directory actually.
Assignee | ||
Updated•16 years ago
|
Whiteboard: [fixed1.9.1b5: Av2a] → [fixed1.9.1b99: Av2a]
Comment 16•8 years ago
|
||
`make check` is still pretty bad to this day. But I don't see much value in keeping this bug open.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•