Closed Bug 702388 Opened 13 years ago Closed 12 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla13

People

(Reporter: gps, Assigned: panagiotis.koutsourakis, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 1 obsolete file)

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!
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! :-)
Assignee: gps → nobody
Summary: Convert Makefiles to use TEST_DIRS → Convert Makefiles to use |TEST_DIRS += foo| instead of |ifdef ENABLE_TESTS \n DIRS += foo|
Whiteboard: [good first bug] [mentor=edmorley]
Looks like there is a proposal for MOCHITESTS_DIR in bug 370750. If that bug lands, that will impact this one.
Depends on: 370750
Blocks: 697894
(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.
(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.
(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.
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.
(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.
Assignee: nobody → panagiotis.koutsourakis
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.
Attachment #590455 - Flags: review?(khuey)
(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 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.
Attachment #590455 - Flags: review?(khuey) → review+
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! :-)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla13
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 :-)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Thanks :) I'll take you up on that :)
There also  some in layout\Makefile.in.

Er, Should change it?
As per our discussion with Kyle in #developers I did not change layout/Makefile.in
Attachment #595341 - Flags: review?(khuey)
Also, I've just spotted that bug 294260 comment 53 (cset 21cc56ed5fe6) removed a TEST_DIRS accidentally :-(

(Reopening for the second patch).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I've said to Matt that you would be ok including the comment 16 change in your followup if that's ok? :-)
This patch includes the file mentioned in #comment 16.
Attachment #595341 - Attachment is obsolete: true
Attachment #595341 - Flags: review?(khuey)
Attachment #595690 - Flags: review?(khuey)
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
Attachment #595690 - Flags: review?(khuey) → review+
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
https://hg.mozilla.org/mozilla-central/rev/1f30e16bf02d
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Mentor: emorley
Whiteboard: [good first bug] [mentor=edmorley] → [good first bug]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.