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)
Firefox Build System
General
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 hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review |
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
Assignee | ||
Updated•7 years ago
|
Attachment #8871834 -
Flags: review?(nfroyd)
Comment 4•7 years ago
|
||
(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)
Assignee | ||
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
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....
Assignee | ||
Comment 7•7 years ago
|
||
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).
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Summary: Add an option --enable-diagnostic-assert and use it → Enable the diagnostic assert when MOZ_DEV_EDITION is set
Comment 14•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 15•7 years ago
|
||
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 16•7 years ago
|
||
mozreview-review |
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-
Comment 17•7 years ago
|
||
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.
Assignee | ||
Comment 18•7 years ago
|
||
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)
Updated•7 years ago
|
status-firefox57:
affected → ---
Assignee | ||
Comment 19•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
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 22•7 years ago
|
||
mozreview-review |
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+
Comment 23•7 years ago
|
||
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
Comment 24•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 26•7 years ago
|
||
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
Updated•6 years ago
|
Component: Build Config → General
Product: Firefox → Firefox Build System
Updated•6 years ago
|
Target Milestone: Firefox 55 → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•