Last Comment Bug 702388 - Convert Makefiles to use |TEST_DIRS += foo| instead of |ifdef ENABLE_TESTS \n DIRS += foo|
: Convert Makefiles to use |TEST_DIRS += foo| instead of |ifdef ENABLE_TESTS \n...
Status: RESOLVED FIXED
[good first bug]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Panagiotis Koutsourakis
:
Mentors: Ed Morley [:emorley]
Depends on: 370750 701822
Blocks: 697894 735433
  Show dependency treegraph
 
Reported: 2011-11-14 12:03 PST by Gregory Szorc [:gps]
Modified: 2014-06-25 01:06 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Change Makefiles to use the variable TEST_DIRS (45.58 KB, patch)
2012-01-21 04:36 PST, Panagiotis Koutsourakis
khuey: review+
Details | Diff | Review
Convert some more files ommited in the previous patch. (2.80 KB, patch)
2012-02-08 01:14 PST, Panagiotis Koutsourakis
no flags Details | Diff | Review
Convert some more files ommited in the previous patch. (3.25 KB, patch)
2012-02-09 01:48 PST, Panagiotis Koutsourakis
khuey: review+
Details | Diff | Review

Description Gregory Szorc [:gps] 2011-11-14 12:03:49 PST
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] 2011-11-14 12:27:39 PST
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] 2011-11-18 11:40:29 PST
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: gavin@gavinsharp.com] 2011-11-23 15:00:48 PST
(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 2012-01-20 13:09:42 PST
(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] 2012-01-20 13:30:28 PST
(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] 2012-01-20 13:36:07 PST
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 2012-01-20 13:37:47 PST
(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 2012-01-21 04:36:43 PST
Created attachment 590455 [details] [diff] [review]
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] 2012-01-22 04:33:22 PST
(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 [:khuey] (khuey@mozilla.com) 2012-01-30 03:10:22 PST
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] 2012-02-04 09:35:35 PST
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] 2012-02-05 04:06:49 PST
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 2012-02-05 08:17:44 PST
Thanks :) I'll take you up on that :)
Comment 14 fly3ds@qq.com 2012-02-05 20:42:35 PST
There also  some in layout\Makefile.in.

Er, Should change it?
Comment 15 Panagiotis Koutsourakis 2012-02-08 01:14:14 PST
Created attachment 595341 [details] [diff] [review]
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] 2012-02-08 04:24:49 PST
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] 2012-02-08 14:43:29 PST
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 2012-02-09 01:48:38 PST
Created attachment 595690 [details] [diff] [review]
Convert some more files ommited in the previous patch.

This patch includes the file mentioned in #comment 16.
Comment 19 Panagiotis Koutsourakis 2012-02-10 06:48:51 PST
Sent to try server: https://tbpl.mozilla.org/?tree=Try&rev=6c114e3b8c55
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-10 12:28:37 PST
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] 2012-02-10 13:11:51 PST
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] 2012-02-10 19:19:16 PST
https://hg.mozilla.org/mozilla-central/rev/1f30e16bf02d

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