Last Comment Bug 781498 - TESTING_JS_MODULES should be conditional on ENABLE_TESTS
: TESTING_JS_MODULES should be conditional on ENABLE_TESTS
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- minor (vote)
: mozilla18
Assigned To: Gregory Szorc [:gps]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-09 05:54 PDT by Ryan VanderMeulen [:RyanVM]
Modified: 2012-09-13 13:36 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add missing ifdef (1.03 KB, patch)
2012-08-09 05:55 PDT, Ryan VanderMeulen [:RyanVM]
gps: review-
Details | Diff | Review
directory contents (13.66 KB, image/png)
2012-09-08 12:28 PDT, Ryan VanderMeulen [:RyanVM]
no flags Details
Make rule conditional, v1 (2.21 KB, patch)
2012-09-08 12:52 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Review
Make rule conditional, v2 (1.67 KB, patch)
2012-09-08 12:55 PDT, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Review

Description Ryan VanderMeulen [:RyanVM] 2012-08-09 05:54:23 PDT
They shouldn't be copied if tests are disabled.
Comment 1 Ryan VanderMeulen [:RyanVM] 2012-08-09 05:55:21 PDT
Created attachment 650519 [details] [diff] [review]
Add missing ifdef
Comment 2 Ryan VanderMeulen [:RyanVM] 2012-08-09 11:42:13 PDT
Green on Try (the failures on that push are from one of the other patches).
Comment 3 Ryan VanderMeulen [:RyanVM] 2012-08-09 11:43:37 PDT
(In reply to Ryan VanderMeulen from comment #2)
> Green on Try (the failures on that push are from one of the other patches).

https://tbpl.mozilla.org/?tree=Try&rev=04641e43950a
Comment 4 Gregory Szorc [:gps] 2012-08-09 12:57:02 PDT
Comment on attachment 650519 [details] [diff] [review]
Add missing ifdef

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

This is being addressed in bug 781307.
Comment 5 Gregory Szorc [:gps] 2012-08-09 12:57:50 PDT

*** This bug has been marked as a duplicate of bug 781307 ***
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-09-08 08:40:05 PDT
This was not fixed by bug 781307. I still see these in my objdir on a clean build from tip.
Comment 7 Gregory Szorc [:gps] 2012-09-08 10:48:30 PDT
I just looked at services/common/Makefile.in and it is doing everything right. If what you are saying is true, this is a bug in rules.mk.

Exactly what files are you seeing copied into the objdir?
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-09-08 12:28:14 PDT
Created attachment 659512 [details]
directory contents

Here's what's in there. Looks like the directory is created at the very end of the build process (after linking libxul).
Comment 9 Gregory Szorc [:gps] 2012-09-08 12:49:00 PDT
This is a core build system bug. The rule in rules.mk is not conditional on ENABLE_TESTS.
Comment 10 Gregory Szorc [:gps] 2012-09-08 12:52:43 PDT
Created attachment 659517 [details] [diff] [review]
Make rule conditional, v1

Trivial patch.
Comment 11 Gregory Szorc [:gps] 2012-09-08 12:55:03 PDT
Created attachment 659518 [details] [diff] [review]
Make rule conditional, v2

On second thought, I'm pretty sure packaging may break if the testing modules directory isn't created. This is a safer patch.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-09-08 12:59:19 PDT
My original patch led to no _tests directory being created in the objdir, and I didn't have any issues with packaging that I could see.
Comment 13 Gregory Szorc [:gps] 2012-09-08 13:05:19 PDT
(In reply to Ryan VanderMeulen from comment #12)
> My original patch led to no _tests directory being created in the objdir,
> and I didn't have any issues with packaging that I could see.

The ifdefs for this should be in rules.mk, not in individual Makefile.in. Even if you fixed services/common/Makefile.in, there's still an instance in netwerk/ that would be populating files.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-09-08 13:16:03 PDT
1) |ifdef ENABLE_TESTS| is used all over the tree, so I don't fully understand your comment. [1]
2) Regardless, the point I was attempting to make was that in response to comment 11, I didn't observe any loss of functionality during packaging when testing my original patch (which at least at the time led to no _tests directory being created in the objdir).

[1] http://mxr.mozilla.org/mozilla-central/search?string=ifdef+ENABLE_TESTS&find=%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Comment 15 Gregory Szorc [:gps] 2012-09-08 13:18:56 PDT
(In reply to Ryan VanderMeulen from comment #14)
> 1) |ifdef ENABLE_TESTS| is used all over the tree, so I don't fully
> understand your comment. [1]

We are trying to move away from that convention by creating variables that have TEST embedded in their name. This minimizes needed logic in Makefile.in.

> 2) Regardless, the point I was attempting to make was that in response to
> comment 11, I didn't observe any loss of functionality during packaging when
> testing my original patch (which at least at the time led to no _tests
> directory being created in the objdir).

Maybe. I don't want to take the chance. I burned all the trees as part of landing testing-only JS modules because of packaging BS. I'd rather not risk it again.
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-09-08 13:24:31 PDT
(In reply to Gregory Szorc [:gps] from comment #15)
> We are trying to move away from that convention by creating variables that
> have TEST embedded in their name. This minimizes needed logic in Makefile.in.
> 

That'll be a nice improvement! Is there a bug tracking this work?

> Maybe. I don't want to take the chance. I burned all the trees as part of
> landing testing-only JS modules because of packaging BS. I'd rather not risk
> it again.

Just for kicks, I pushed v1 to Try to see what happens.
https://tbpl.mozilla.org/?tree=Try&rev=9d4d45e1ddc8
Comment 17 Gregory Szorc [:gps] 2012-09-08 13:29:32 PDT
(In reply to Ryan VanderMeulen from comment #16)
> (In reply to Gregory Szorc [:gps] from comment #15)
> > We are trying to move away from that convention by creating variables that
> > have TEST embedded in their name. This minimizes needed logic in Makefile.in.
> > 
> 
> That'll be a nice improvement! Is there a bug tracking this work?

Long term this will get address by converting the Makefile.in to Python scripts. The start of that work is being tracked in bug 784841.

> > Maybe. I don't want to take the chance. I burned all the trees as part of
> > landing testing-only JS modules because of packaging BS. I'd rather not risk
> > it again.
> 
> Just for kicks, I pushed v1 to Try to see what happens.
> https://tbpl.mozilla.org/?tree=Try&rev=9d4d45e1ddc8

I should have been more clear. Packaging packaging will work. However, the test runners may fail attempting to extract a non-present directory in the package archive. It's an overly complicated mess.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-09-08 13:40:39 PDT
(In reply to Gregory Szorc [:gps] from comment #17)
> Long term this will get address by converting the Makefile.in to Python
> scripts. The start of that work is being tracked in bug 784841.
> 

OK, I'm already watching that bug. Makes sense to not duplicate efforts.

> I should have been more clear. Packaging packaging will work. However, the
> test runners may fail attempting to extract a non-present directory in the
> package archive. It's an overly complicated mess.

Fair enough. https://tbpl.mozilla.org/?tree=Try&rev=6e8f4aa631fe :)
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-09-08 14:38:00 PDT
Of course, now I'm realizing the silliness of this conversation. ENABLE_TESTS will be true if we're building with tests enabled. If we --disable-tests, I would expect the test runners to puke no matter what we do here. Since --disable-tests is NPOTB, the patch in this bug should have no effect on Tinderbox builds.
Comment 20 Gregory Szorc [:gps] 2012-09-08 14:59:20 PDT
(In reply to Ryan VanderMeulen from comment #19)
> Of course, now I'm realizing the silliness of this conversation.
> ENABLE_TESTS will be true if we're building with tests enabled. If we
> --disable-tests, I would expect the test runners to puke no matter what we
> do here. Since --disable-tests is NPOTB, the patch in this bug should have
> no effect on Tinderbox builds.

Don't worry about it: the build system makes everyone look silly at times, build peers included.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-09-10 07:06:38 PDT
So wait, is v2 still the way to go given that this is NPOTB?
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-09-10 18:44:35 PDT
https://hg.mozilla.org/mozilla-central/rev/f917cdb9b3ea
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-09-13 13:36:50 PDT
Verified no _tests directory in the objdir now. Thanks!

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