488091 - Review |make check| usages

Status: RESOLVED INCOMPLETE

(Firefox Build System :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

Nits or fixes found while checking bug 485736.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Remove empty targets, add ifdef, use PARRALEL_DIRS

Passed on Tryserver as 'try-9534e0d4ded'.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS [Checkin: See comment 5]

Untested, but trivial.

Comment 3

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 372587 [details] [diff] [review]
(Bv1-CC) </calendar/>: Remove empty targets, use PARRALEL_DIRS
[Checkin: See comment 5]

r=philipp

Comment 4

[(not currently active) Ted Mielczarek]

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.

Comment 5

[Serge Gautherie (:sgautherie)]

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.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Remove empty targets, add ifdef [Checkin: See comment 10 & 11]

Av1, with comment 4 suggestion(s).

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) </security/>: Remove empty targets, add ifdef

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?

Comment 8

[Wan-Teh Chang]

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

Comment 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

[Serge Gautherie (:sgautherie)]

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).

Comment 11

[Serge Gautherie (:sgautherie)]

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

Comment 12

[Honza Bambas (:mayhemer)]

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?

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2) </security/>: update shortcut, add ifdef, remove empty targets

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.

Comment 14

[Wan-Teh Chang]

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.

Comment 15

[Serge Gautherie (:sgautherie)]

(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.

Comment 16

[Gregory Szorc [:gps]]

`make check` is still pretty bad to this day. But I don't see much value in keeping this bug open.