781498 - TESTING_JS_MODULES should be conditional on ENABLE_TESTS

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

They shouldn't be copied if tests are disabled.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: Add missing ifdef

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Green on Try (the failures on that push are from one of the other patches).

Comment 3

[Ryan VanderMeulen [:RyanVM]]

(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]]

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 6

[Ryan VanderMeulen [:RyanVM]]

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]]

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]]

Attachment: 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]]

This is a core build system bug. The rule in rules.mk is not conditional on ENABLE_TESTS.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Make rule conditional, v1

Trivial patch.

Comment 11

[Gregory Szorc [:gps]]

Attachment: 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]]

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]]

(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]]

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]]

(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]]

(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]]

(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]]

(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]]

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]]

(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]]

So wait, is v2 still the way to go given that this is NPOTB?

Comment 22

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f917cdb9b3ea

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f917cdb9b3ea

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Verified no _tests directory in the objdir now. Thanks!