702388 - Convert Makefiles to use |TEST_DIRS += foo| instead of |ifdef ENABLE_TESTS \n DIRS += foo|

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Now that we have a TEST_DIRS variable (bug 701822), the Makefiles should make use of it.

Basically, this involves replacing:

  ifdef ENABLE_TESTS
  DIRS += tests
  endif

With

  TEST_DIRS += tests

Since I created this mess, I'll mop it up. Not sure when I'll get to it though. If someone wants to have a go at it, please do!

Comment 1

[Ed Morley [:emorley]]

For anyone wanting to do this as a first bug:

1) Get mozilla-central:
   https://developer.mozilla.org/En/Developer_Guide/Source_Code/Mercurial

2) Make the change mentioned in comment 0 for each of the search results here:
   http://mxr.mozilla.org/mozilla-central/search?string=ifdef+ENABLE_TESTS&find=Makefile\.in

3) Check everything still builds ok:
   https://developer.mozilla.org/En/Simple_Firefox_build
   ...NB: Make sure bug 701822 has been marked as fixed (ie has merged to mozilla-central), since until it does, TEST_DIRS won't work.

4) Create a patch to attach here:
   https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch
   https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f

5) Set the r? flag to request review from Kyle Huey when you attach the patch (type in :khuey and it will auto-complete).

If you have any questions (or would like to take on this bug and don't have the needed bugzilla permissions to assign it to yourself), just ask! :-)

Comment 2

[Gregory Szorc [:gps]]

Looks like there is a proposal for MOCHITESTS_DIR in bug 370750. If that bug lands, that will impact this one.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to Gregory Szorc [:gps] from comment #2)
> Looks like there is a proposal for MOCHITESTS_DIR in bug 370750

Note that the latest patch in that bug uses creates *_FILES, not *_DIR, so it's somewhat orthogonal.

Comment 4

[Panagiotis Koutsourakis]

(In reply to Ed Morley [:edmorley] from comment #1)

> 2) Make the change mentioned in comment 0 for each of the search results
> here:
>   
> http://mxr.mozilla.org/mozilla-central/
> search?string=ifdef+ENABLE_TESTS&find=Makefile\.in
> 

Most of the files in that set do not contain the string 
  DIRS += tests 
(I found only 27). For instance rdf/Makefile.in has
  TOOL_DIRS += tests
I guess all of them should be changed, but I'd like to verify this first.

Also I'm not sure how I can take ownership of this bug. I just created an account so if someone would be kind enough to help me with this I would appreciate it very much.

Comment 5

[Gregory Szorc [:gps]]

(In reply to Panagiotis Koutsourakis from comment #4)
> Also I'm not sure how I can take ownership of this bug. I just created an
> account so if someone would be kind enough to help me with this I would
> appreciate it very much.

The bug is is assigned to "nobody," which means it is free for someone to take. If you are intent on addressing this bug, assign the bug to yourself.

Comment 6

[Gregory Szorc [:gps]]

Doh. Apparently new Bugzilla users can't assign bugs to themselves. So, if you want to do the work for this bug, just say so and someone (likely this bug's mentor - Ed Morley) will assign it to you. Sorry for the mix-up.

Comment 7

[Panagiotis Koutsourakis]

(In reply to Gregory Szorc [:gps] from comment #6)
> Doh. Apparently new Bugzilla users can't assign bugs to themselves. So, if
> you want to do the work for this bug, just say so and someone (likely this
> bug's mentor - Ed Morley) will assign it to you. Sorry for the mix-up.

No problem. Yes, I'd like to work on it. Thanks for the help.

Comment 8

[Panagiotis Koutsourakis]

Attachment: Change Makefiles to use the variable TEST_DIRS

As mentioned in the commit message some files that contain the string

ifdef ENABLE_TESTS remain unchanged by this patch.

The source compiled with no problems under GNU/Linux 3.0.6 with gcc 4.5.3. Unfortunately I don't have access to other platforms (or the try server), so I could not test if it compiles in other platforms.

If further changes are needed, I'd be happy to help.

Comment 9

[Ed Morley [:emorley]]

(In reply to Panagiotis Koutsourakis from comment #8)
> The source compiled with no problems under GNU/Linux 3.0.6 with gcc 4.5.3.
> Unfortunately I don't have access to other platforms (or the try server), so
> I could not test if it compiles in other platforms.

Sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=0856745e4ed2

:-)

Comment 10

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

Comment on attachment 590455 [details] [diff] [review]
Change Makefiles to use the variable TEST_DIRS

Review of attachment 590455 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Thanks for the patch.

Comment 11

[Ed Morley [:emorley]]

Sorry for the delay, this got lost in my bugmails.

Have pushed to mozilla-inbound, which will get merged to mozilla-central within the next 24 hours.

https://hg.mozilla.org/integration/mozilla-inbound/rev/064aba8b26f2

Thanks for the patch! :-)

Comment 12

[Ed Morley [:emorley]]

Merged to mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/064aba8b26f2

If you'd like any inspiration for things to work on next - pop onto #developers on irc.mozilla.org and we'll gladly find something for you to work on :-)

Comment 13

[Panagiotis Koutsourakis]

Thanks :) I'll take you up on that :)

Comment 14

[[e-mail]]

There also  some in layout\Makefile.in.

Er, Should change it?

Comment 15

[Panagiotis Koutsourakis]

Attachment: Convert some more files ommited in the previous patch.

As per our discussion with Kyle in #developers I did not change layout/Makefile.in

Comment 16

[Ed Morley [:emorley]]

Also, I've just spotted that bug 294260 comment 53 (cset 21cc56ed5fe6) removed a TEST_DIRS accidentally :-(

(Reopening for the second patch).

Comment 17

[Ed Morley [:emorley]]

I've said to Matt that you would be ok including the comment 16 change in your followup if that's ok? :-)

Comment 18

[Panagiotis Koutsourakis]

Attachment: Convert some more files ommited in the previous patch.

This patch includes the file mentioned in #comment 16.

Comment 19

[Panagiotis Koutsourakis]

Sent to try server: https://tbpl.mozilla.org/?tree=Try&rev=6c114e3b8c55

Comment 20

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

Comment on attachment 595690 [details] [diff] [review]
Convert some more files ommited in the previous patch.

Review of attachment 595690 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 21

[Ed Morley [:emorley]]

TBPL looks good to me (I haven't starred, since I thought you might want to get used to it :-))
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f30e16bf02d

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f30e16bf02d