TESTING_JS_MODULES should be conditional on ENABLE_TESTS

VERIFIED FIXED in mozilla18

Status

()

Core
Build Config
--
minor
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: RyanVM, Assigned: gps)

Tracking

unspecified
mozilla18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

They shouldn't be copied if tests are disabled.
Created attachment 650519 [details] [diff] [review]
Add missing ifdef
Attachment #650519 - Flags: review?(gps)
Green on Try (the failures on that push are from one of the other patches).
(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
(Assignee)

Comment 4

5 years ago
Comment on attachment 650519 [details] [diff] [review]
Add missing ifdef

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

This is being addressed in bug 781307.
Attachment #650519 - Flags: review?(gps) → review-
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 781307
This was not fixed by bug 781307. I still see these in my objdir on a clean build from tip.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 7

5 years ago
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?
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).
(Assignee)

Comment 9

5 years ago
This is a core build system bug. The rule in rules.mk is not conditional on ENABLE_TESTS.
Assignee: ryanvm → nobody
Status: REOPENED → NEW
Component: Firefox: Common → Build Config
Product: Mozilla Services → Core
Summary: Common services test files are copied unconditionally to the objdir. → TESTING_JS_MODULES should be conditional on ENABLE_TESTS
(Assignee)

Comment 10

5 years ago
Created attachment 659517 [details] [diff] [review]
Make rule conditional, v1

Trivial patch.
Assignee: nobody → gps
Attachment #650519 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #659517 - Flags: review?(mh+mozilla)
(Assignee)

Comment 11

5 years ago
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.
Attachment #659517 - Attachment is obsolete: true
Attachment #659517 - Flags: review?(mh+mozilla)
Attachment #659518 - Flags: review?(mh+mozilla)
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.
(Assignee)

Comment 13

5 years ago
(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.
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
(Assignee)

Comment 15

5 years ago
(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.
(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
(Assignee)

Comment 17

5 years ago
(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.
(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 :)
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.
(Assignee)

Comment 20

5 years ago
(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.
Attachment #659518 - Flags: review?(mh+mozilla) → review+
So wait, is v2 still the way to go given that this is NPOTB?
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f917cdb9b3ea
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/f917cdb9b3ea
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Verified no _tests directory in the objdir now. Thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.