Closed Bug 1773259 Opened 2 years ago Closed 2 years ago

Snap for 102.0b5 fails to build with "error: redefinition of 'ROOT_CLIP_CHAIN'" with cbindgen > 0.23

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Version:
Firefox 102
Platform:
Desktop
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr91 --- wontfix
firefox-esr102 108+ fixed
firefox101 --- wontfix
firefox102 --- wontfix
firefox103 --- unaffected
firefox104 --- unaffected

People

(Reporter: olivier, Assigned: glandium)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1773259 - Work around build failure with newer cbindgen.
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-esr102+
Details | Review

Automated builds for the snap succeeded for version 102.0b4-1, and are now failing for version 102.0b5-2 with the following build error:

1:20.70 In file included from Unified_cpp_accessible_base0.cpp:119:
1:20.70 In file included from /build/firefox/parts/firefox/build/accessible/base/FocusManager.cpp:21:
1:20.70 In file included from /build/firefox/parts/firefox/build/obj-x86_64-pc-linux-gnu/instrumented/dist/include/mozilla/dom/BrowserParent.h:23:
1:20.70 In file included from /build/firefox/parts/firefox/build/obj-x86_64-pc-linux-gnu/instrumented/dist/include/mozilla/layout/RemoteLayerTreeOwner.h:17:
1:20.70 In file included from /build/firefox/parts/firefox/build/obj-x86_64-pc-linux-gnu/instrumented/dist/include/nsDisplayList.h:48:
1:20.70 In file included from /build/firefox/parts/firefox/build/obj-x86_64-pc-linux-gnu/instrumented/dist/include/nsCSSRenderingBorders.h:20:
1:20.71 In file included from /build/firefox/parts/firefox/build/obj-x86_64-pc-linux-gnu/instrumented/dist/include/gfxUtils.h:23:
1:20.71 In file included from /build/firefox/parts/firefox/build/obj-x86_64-pc-linux-gnu/instrumented/dist/include/mozilla/webrender/WebRenderTypes.h:11:
1:20.71 In file included from /build/firefox/parts/firefox/build/obj-x86_64-pc-linux-gnu/instrumented/dist/include/mozilla/webrender/webrender_ffi.h:104:
1:20.71 /build/firefox/parts/firefox/build/obj-x86_64-pc-linux-gnu/instrumented/dist/include/mozilla/webrender/webrender_ffi_generated.h:24:33: error: redefinition of 'ROOT_CLIP_CHAIN'
1:20.71 constexpr static const uint64_t ROOT_CLIP_CHAIN = ~0;
1:20.71                                 ^
1:20.71 /build/firefox/parts/firefox/build/obj-x86_64-pc-linux-gnu/instrumented/dist/include/mozilla/webrender/webrender_ffi.h:76:16: note: previous definition is here
1:20.71 const uint64_t ROOT_CLIP_CHAIN = ~0;
1:20.71                ^
1:22.63 1 error generated.
1:22.65 make[4]: *** [/build/firefox/parts/firefox/build/config/rules.mk:660: Unified_cpp_accessible_base0.o] Error 1
1:22.65 make[3]: *** [/build/firefox/parts/firefox/build/config/recurse.mk:72: accessible/base/target-objects] Error 2

I see that the extra definition was removed as part of commit 51947744ce12247f378a1db2379ffaad3fcd18c3, but the commit message isn't explicit why.

Emilio, you authored the commit that removes the extra definition of ROOT_CLIP_CHAIN, can you help me understand why this regressed in the first place, and if cherry-picking that single removal is a correct way to fix the problem?

Why is this not affecting non-snap builds?

Flags: needinfo?(emilio)
Blocks: snap

Yes, I removed it because the new cbindgen version is smart enough to generate it. Removing it should be correct.

Flags: needinfo?(emilio)

Oh right, the regression happened because version 0.24.2 of cbindgen was used to build the new beta snap, where it was using version 0.23.0 for previous builds.

I also hit that on nightly

Tentativaly fixed with https://github.com/canonical/firefox-snap/commit/5622734942524846fb0eb7108918c8cd8557fde3, waiting for builds to complete to confirm.

Component: General → Widget: Gtk
Product: Firefox → Core
Regressed by: 1773070

Set release status flags based on info from the regressing bug 1773070

status-firefox101: --- → unaffected
status-firefox102: --- → unaffected
status-firefox103: --- → affected
status-firefox-esr91: --- → unaffected

I confirm the minimal patch in comment 5 fixes the snap builds with cbindgen 0.24.2. I had to cherry-pick the patch for all the snap branches (stable, beta, esr).

Yeah, pretty sure this is invalid. The tool became smart enough to generate something that was written down manually before. It's a shame that the tool conflicted with it, but other than backporting comment 5 + tool updates there's not all that much to do here unfortunately...

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INVALID
Has Regression Range: --- → yes

Fwiw this also affects building 101 with cbindgen 0.24.3.

And 91.10.0 from the mozilla-esr91 branch ... so should there be a backport there too ?

Flags: needinfo?(emilio)
status-firefox101: unaffected → affected
status-firefox102: unaffected → fixed
status-firefox-esr91: unaffected → affected
Summary: Snap for 102.0b5 fails to build with "error: redefinition of 'ROOT_CLIP_CHAIN'" → Snap for 102.0b5 fails to build with "error: redefinition of 'ROOT_CLIP_CHAIN'" with cbindgen > 0.23

given that there's no control over which cbindgen maximal version is used to build beta/release/esr, reopening so that it stays on the radar ?

Status: RESOLVED → REOPENED
status-firefox103: affected → unaffected
Resolution: INVALID → ---

I mean, not sure realistically what the best way to deal with this other than "just remove that line I'd you're building with a newer tool" would be... Mike, thoughts? Tldr: newer cbindgen versions are smart enough to generate a constant that WR folks defined manually (ugh), so using a too new cbindgen with an old tree causes an error like comment 0.

The only needed thing is comment 5 (or an equivalent cbindgen.toml patch to avoid generating that constant). Making cbindgen not generate negations by default would be quite unfortunate.

We could update cbindgen and land comment 5 in all trees I guess, is that reasonable?

Flags: needinfo?(emilio) → needinfo?(mh+mozilla)

sadly, building thunderbird 91.10.0 with cbindgen 0.24.3 also fails. Too bad its a constant and not a #define, otherwise we could have gotten away with it via #ifdefs...

We could update cbindgen and land comment 5 in all trees I guess, is that reasonable?

That would bump the cbindgen requirement, which is not necessarily very easy to update. (BTW, we should probably make the base toolchain jobs use the oldest supported cbindgen)

I guess #ifdef'ing out the const when an "older" version of cbindgen is used would help.

Flags: needinfo?(mh+mozilla)

(BTW, we should probably make the base toolchain jobs use the oldest supported cbindgen)

Well, I guess we're always doing that. The problem is that new versions of cbindgen sometimes break us, and the way we "fix" it is to upgrade the requirement... which doesn't work for older versions of the gecko source.

Set release status flags based on info from the regressing bug 1773070

status-firefox104: --- → affected

If my understanding is right, new versions as/after the version of bug 1773070 are not affected.
status-firefox103 and status-firefox104 are unaffected.
(Emilio, sorry for the ping and appreciate your confirmation here. :) )

Flags: needinfo?(emilio)

Yes, that's right.

Flags: needinfo?(emilio)
Assignee: nobody → mh+mozilla

Comment on attachment 9303810 [details]
Bug 1773259 - Work around build failure with newer cbindgen.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Failure to build downstream with a newer version of cbindgen
  • User impact if declined: See above
  • Fix Landed on Version: N/A
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a no-op for older versions of cbindgen (which we use on our automation for esr).
Attachment #9303810 - Flags: approval-mozilla-esr102?
Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
status-firefox-esr91: affected → wontfix
tracking-firefox-esr102: --- → 108+
Resolution: --- → FIXED

Comment on attachment 9303810 [details]
Bug 1773259 - Work around build failure with newer cbindgen.

Approved for 102.6esr.

Attachment #9303810 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+

This was backed out for breaking Hazard builds.
https://hg.mozilla.org/releases/mozilla-esr102/rev/9b2625d4969500aa892ae6790e46adb3db7af7ea

https://treeherder.mozilla.org/logviewer?job_id=397954559&repo=mozilla-esr102&lineNumber=356

Traceback (most recent call last):
  File "/builds/worker/checkouts/gecko/configure.py", line 349, in <module>
    sys.exit(main(sys.argv))
  File "/builds/worker/checkouts/gecko/configure.py", line 131, in main
    sandbox.run(os.path.join(os.path.dirname(__file__), "moz.configure"))
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 567, in run
    func(*args)
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 618, in _value_for
    return self._value_for_depends(obj)
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/util.py", line 1061, in method_call
    cache[args] = self.func(instance, *args)
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 627, in _value_for_depends
    value = obj.result()
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/util.py", line 1061, in method_call
    cache[args] = self.func(instance, *args)
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 163, in result
    return self._func(*resolved_args)
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 1310, in wrapped
    return new_func(*args, **kwargs)
  File "/builds/worker/checkouts/gecko/build/moz.configure/bindgen.configure", line 98, in <lambda>
    lambda c: Version(check_cmd_output(c, "--version").strip().split(" ")[1])
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 1310, in wrapped
    return new_func(*args, **kwargs)
  File "/builds/worker/checkouts/gecko/build/moz.configure/util.configure", line 57, in check_cmd_output
    retcode, stdout, stderr = get_cmd_output(*args, **kwargs)
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 1310, in wrapped
    return new_func(*args, **kwargs)
  File "/builds/worker/checkouts/gecko/build/moz.configure/util.configure", line 30, in get_cmd_output
    log.debug("Executing: `%s`", quote(*args))
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/shellutil.py", line 209, in quote
    return " ".join(_quote(s) for s in strings)
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/shellutil.py", line 209, in <genexpr>
    return " ".join(_quote(s) for s in strings)
  File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/shellutil.py", line 197, in _quote
    return t("'%s'") % s.replace(t("'"), t("'\\''"))
TypeError: NoneType takes no arguments
status-firefox-esr102: fixed → affected
Flags: needinfo?(mh+mozilla)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)

This was backed out for breaking Hazard builds.
https://hg.mozilla.org/releases/mozilla-esr102/rev/9b2625d4969500aa892ae6790e46adb3db7af7ea

https://treeherder.mozilla.org/logviewer?job_id=397954559&repo=mozilla-esr102&lineNumber=356

Updated the patch. (where did the r=emilio come from in the landing?)

Flags: needinfo?(mh+mozilla) → needinfo?(ryanvm)

Good question! Thanks for updating the patch, I'll fix the reviewer when it re-lands.

Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: