Closed Bug 1368079 Opened 7 years ago Closed 7 years ago

Enable the diagnostic assert when MOZ_DEV_EDITION is set

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Needed for beta/devedition, we need a new option to enable the diagnostic assert. 
The patch adds it and use the new option.
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review147062

I don't think I'm a good reviewer for this, sorry :(
Attachment #8871834 - Flags: review?(bhearsum)
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review147034

::: mfbt/Assertions.h:453
(Diff revision 1)
>  #endif /* DEBUG */
>  
> -#ifdef RELEASE_OR_BETA
> +#if defined(RELEASE_OR_BETA) && !defined(ENABLE_DIAGNOSTIC_ASSERT)
>  #  define MOZ_DIAGNOSTIC_ASSERT MOZ_ASSERT
>  #  ifdef DEBUG
>  #    define MOZ_DIAGNOSTIC_ASSERT_ENABLED 1

Because of this line, I haven't reused MOZ_DIAGNOSTIC_ASSERT_ENABLED as argument for the configure result
Attachment #8871834 - Flags: review?(nfroyd)
(In reply to Sylvestre Ledru [:sylvestre] from comment #0)
> Needed for beta/devedition, we need a new option to enable the diagnostic
> assert. 
> The patch adds it and use the new option.

Why is the RELEASE_OR_BETA not applicable anymore?  Is this because Aurora is effectively going away, and we now need a separate option to indicate "early beta" vs. "late beta"?  Could we just tune RELEASE_OR_BETA to reflect the appropriate condition?
Flags: needinfo?(sledru)
Aurora is going anyway for sure. 
However, devedition is migrated on the beta channel and we have the capability to build it with different options.

We made the call to keep the diagnostic assert just for Devedition on beta during the whole cycle.
With this new option, the goal is to add it to the mozconfig of this specific build.

When we were discussing the option to do only a repack, we were considering to use only RELEASE_OR_BETA but now that we do rebuild, we can enable it only for devedition.

don't hesitate if you have any other questions.
Flags: needinfo?(sledru)
Blocks: 1368172
So now we have RELEASE_OR_BETA, and this other thing that's sort of BETA but is actually built with different options than the real BETA?  That seems like a recipe for bad things happening later....
I agree this isn't a perfect solution. We are discussing on improving this for the future.
Until we have a better way to do it, I believe that my solution is pretty easy for now (for the diagnostic assert).
So, with aurora, we had !RELEASE_OR_BETA && !NIGHTLY_BUILD meaning aurora.

With aurora out of the picture, RELEASE_OR_BETA and NIGHTLY_BUILD are just two opposite values. Seems like RELEASE_OR_BETA could be removed.

Now, about things like MOZ_DIAGNOSTIC_ASSERT, seems to me there should be a more general high-level-view discussion about it. With aurora out of the picture, the current setup with them only enabled on nightly is not really enticing. Having them on devedition, is nice-ish, but there's a case for having them on an even wider population too (beta)... OTOH, with release promotion (if it's still a goal; and I'd argue that with aurora gone, we shouldn't go forward with release promotion, except if RCs are built differently from the "normal" betas), that's also not really possible.

Anyways, for the immediate need of enabling MOZ_DIAGNOSTIC_ASSERT on devedition builds, I'd just change mfbt/Assertions.h to enable it for MOZ_DEV_EDITION.
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review147742
Attachment #8871834 - Flags: review?(mh+mozilla) → review-
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review147756

::: mfbt/Assertions.h:450
(Diff revision 2)
>  #  define MOZ_ASSERT(...) MOZ_RELEASE_ASSERT(__VA_ARGS__)
>  #else
>  #  define MOZ_ASSERT(...) do { } while (0)
>  #endif /* DEBUG */
>  
> -#ifdef RELEASE_OR_BETA
> +#if defined(RELEASE_OR_BETA) && !defined(MOZ_DEV_EDITION)

It feels it would be less circumvoluted if the branches were inverted and the condition became #if defined(NIGHTLY_BUILD) || defined(MOZ_DEV_EDITION)
Attachment #8871834 - Flags: review?(mh+mozilla) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df0ddb9c9bef
Enable the diagnostic assert when MOZ_DEV_EDITION is set r=glandium
Summary: Add an option --enable-diagnostic-assert and use it → Enable the diagnostic assert when MOZ_DEV_EDITION is set
https://hg.mozilla.org/mozilla-central/rev/df0ddb9c9bef
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Backed out in https://hg.mozilla.org/mozilla-central/rev/925230851743 - because people diddle and fiddle around with the effects of their MOZ_DIAGNOSTIC_ASSERT, and they weren't expecting this case, that makes DevEdition fail to build, https://treeherder.mozilla.org/logviewer.html#?job_id=103290718&repo=try

You can do the easy case of building trunk as devedition on try, linux, by just changing /config/milestone.txt from 55.0a1 to 55.0 and pushing with "try: -b o -p linux64-devedition-nightly", but if you want the full certainty of everything on every platform, push on top of https://hg.mozilla.org/try/rev/2e018df13d2044ad6c1b5ec255a4768aa36f4a50 with "try: -b o -p linux-devedition-nightly,linux64-devedition-nightly,macosx64,win32,win64 -u all[-10.6,-Windows XP] -t all[-10.6,-Windows XP]"
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review148502

r-'ing because it sounds like there's more work to be done here; people are conditionally compiling things off `RELEASE_OR_BETA`, I guess, which doesn't match with the condition here?

::: mfbt/Assertions.h:453
(Diff revision 3)
>  #endif /* DEBUG */
>  
> -#ifdef RELEASE_OR_BETA
> +#if defined(NIGHTLY_BUILD) || defined(MOZ_DEV_EDITION)
> +#  define MOZ_DIAGNOSTIC_ASSERT MOZ_RELEASE_ASSERT
> +#  define MOZ_DIAGNOSTIC_ASSERT_ENABLED 1
> +#else // RELEASE_OR_BETA

This comment needs updating now.
Attachment #8871834 - Flags: review?(nfroyd) → review-
Looks like the good cases are

#if defined(DEBUG) || !defined(RELEASE_OR_BETA)
foo = bar(baz);
#endif
MOZ_DIAGNOSTIC_ASSERT(foo)

which is nice, since this will burn them and point them out, but there are also

#if !defined(RELEASE_OR_BETA)
foo = bar(baz);
MOZ_DIAGNOSTIC_ASSERT(foo)
#endif

where a) this won't burn and tell us they aren't working on devedition, and b) we're not getting the benefit of MOZ_DIAGNOSTIC_ASSERT being an assert on debug on beta like it should be.
yeah, for this, I started to replace them here:
https://hg.mozilla.org/mozilla-central/rev/f26f1c5652fc
but I am sure I missed some (this was done by grepping the sources)
Same player try again! 

I fixed some stuff that I missed the first time in bug 1370369 (basically what phil mentioned in comment #17).

and I proposed again the same patch (just removed the comment)
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review150178
Attachment #8871834 - Flags: review?(nfroyd) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/592139f3c7ba
Enable the diagnostic assert when MOZ_DEV_EDITION is set r=froydnj,glandium
https://hg.mozilla.org/mozilla-central/rev/592139f3c7ba
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Someone should probably announce on dev-platform that diagnostic asserts are now turned on for dev edition again.  It may not be obvious to people who haven't seen this bug.
It might also be helpful to document the current state for reference on the build defines page:

https://wiki.mozilla.org/Platform/Channel-specific_build_defines
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: