875342 - TypedArray move and parallel js are enabled on non-official builds that are not intended to have them enabled

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

The Makefile.in snippet is:

  ifeq (,$(filter beta release esr,$(MOZ_UPDATE_CHANNEL)))
  DEFINES += -DENABLE_TYPEDARRAY_MOVE
  endif
  ifeq (,$(filter aurora beta release esr,$(MOZ_UPDATE_CHANNEL)))
  DEFINES += -DENABLE_PARALLEL_JS
  endif

MOZ_UPDATE_CHANNEL is usually default for non-official builds of any branch, which means both are enabled on such builds.

This affects release, where the snippet is
  ifeq (,$(filter beta release esr,$(MOZ_UPDATE_CHANNEL)))
  DEFINES += -DENABLE_TYPEDARRAY_MOVE
  endif

and beta, where it is
  ifeq (,$(filter beta release esr,$(MOZ_UPDATE_CHANNEL)))
  DEFINES += -DENABLE_TYPEDARRAY_MOVE
  DEFINES += -DENABLE_PARALLEL_JS
  endif

On other branches, it's supposed to be enabled, so it's not really a problem.

Comment 1

[Phil Ringnalda (:philor)]

Isn't it more serious than that, since on Aurora we set the channel on nightly builds and don't on on-push builds, so that sets us up for nightly-only failures, and on beta and release we set the channel on release builds, but not on on-push builds and there are no nightly builds, so that sets us up for totally unseen release failures. 

I'd think that means that the last time we actually ran tests on the disabled state that we ship was when those branches last built a nightly on Aurora.

Comment 2

[Mike Hommey [:glandium]]

Note there are many other places where MOZ_UPDATE_CHANNEL is used this way. I guess we have to change most of them, and forbid the use of MOZ_UPDATE_CHANNEL, it's just too much of a footgun.

https://mxr.mozilla.org/mozilla-central/search?string=MOZ_UPDATE_CHANNEL

Comment 3

[:Gavin Sharp [email: [e-mail]]]

We should certainly use RELEASE_BUILD/NIGHTLY_BUILD where appropriate, instead of MOZ_UPDATE_CHANNEL.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

See https://wiki.mozilla.org/Platform/Channel-specific_build_defines

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 753885 [details] [diff] [review]
patch

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

Looks right to me.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 753885 [details] [diff] [review]
patch

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

::: js/src/builtin/TestingFunctions.cpp
@@ -168,5 @@
>  #endif
>      if (!JS_SetProperty(cx, info, "oom-backtraces", &value))
>          return false;
>  
> -#ifdef ENABLE_PARALLEL_JS

There's value in keeping ENABLE_PARALLEL_JS: you're not going to forget one when you want it to start riding the train.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Good point, here's a patch that addresses that.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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