Closed Bug 1775185 Opened 2 years ago Closed 2 years ago

Regenerate the Viaduct protobuf classes

Categories

(Firefox :: Sync, task)

Product:
Firefox
Component:
Sync
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
103 Branch
Tracking Flags:
Tracking Status
firefox103 --- fixed

People

(Reporter: RyanVM, Assigned: RyanVM)

References

Details

Attachments

(2 files)

Bug 1775185 - Regenerate Viaduct protobuf classes. r=markh
48 bytes, text/x-phabricator-request
Details | Review
Bug 1775185 - Add explicit keyword back to lines where it was removed during regeneration. r=markh
48 bytes, text/x-phabricator-request
Details | Review

I was recently messing around with our in-tree copy of protobuf and hit Werror bustage in ViaductRequest.cpp due to a missing switch case:
https://treeherder.mozilla.org/logviewer?job_id=381927591&repo=try&lineNumber=36990

After a bit of investigation, I was able to trace it back to when bug 1772070 updated our vendored copy of application-services, which brought along with it the changes from upstream PR #4751. When we run the protobuf class regeneration script, it copies over the vendored copy of fetch_msg_types.proto from the third_party directory to the Viaduct directory in toolkit:
https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/protobuf/regenerate_cpp_files.sh#l28

When we updated A-S, we should have also regenerated the protobuf classes to pick up this change. I'm not sure what the implications of this oversight are in practice, but the fix is straightforward. Longer-term, it would be nice if we could more loudly fail the build or something if fetch_msg_types.proto is out of sync between third_party and toolkit.

Pushed by mhammond@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b63bba7f0db8
Regenerate Viaduct protobuf classes. r=markh

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

Longer-term, it would be nice if we could more loudly fail the build or something if fetch_msg_types.proto is out of sync between third_party and toolkit.

Documenting for posterity, one suggestion glandium had was to 1) eliminate the extra copy of fetch_msg_types.proto and tweak the regeneration script to use the vendored copy directly and 2) add a lint which generates the files from the in-tree proto files and compares with the checked-in version of those generated files as a way of checking for forgotten class regeneration after updating proto files.

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

Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox103: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

Hmm, I got this error with --enable-warnings-as-errors on Windows.

12:31.74 toolkit/system/windowsPackageManager
12:32.45 toolkit/system/windowsproxy
12:32.62 In file included from Unified_cpp_components_viaduct0.cpp:2:
12:32.62 In file included from M:/src/toolkit/components/viaduct/Viaduct.cpp:8:
12:32.63 In file included from M:/fx64-dbg/dist/include\mozilla/ViaductRequest.h:9:
12:32.63 M:/fx64-dbg/dist/include\mozilla/fetch_msg_types.pb.h(129,3): error: bad implicit conversion constructor for 'Request_HeadersEntry_DoNotUse'
12:32.63   Request_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena);
12:32.63   ^
12:32.63 M:/fx64-dbg/dist/include\mozilla/fetch_msg_types.pb.h(129,3): note: consider adding the explicit keyword to the constructor
12:32.63   Request_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena);
12:32.63   ^
12:32.63   explicit
12:32.63 M:/src/toolkit/components/telemetry/other/TelemetryIOInterposeObserver.cpp(133,5): warning: The fully qualified types are preferred over the shorthand typedefs for JS::Handle/JS::Rooted types outside SpiderMonkey.
12:32.63     JS::RootedObject jsStats(cx, JS::NewArrayObject(cx, stats));
12:32.63     ^~~~~~~~~~~~~~~~
12:32.63     JS::Rooted<JSObject*>
12:32.63 M:/fx64-dbg/dist/include\mozilla/fetch_msg_types.pb.h(447,3): error: bad implicit conversion constructor for 'Response_HeadersEntry_DoNotUse'
12:32.63   Response_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena);
12:32.63   ^
12:32.63 M:/fx64-dbg/dist/include\mozilla/fetch_msg_types.pb.h(447,3): note: consider adding the explicit keyword to the constructor
12:32.63   Response_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena);
12:32.63   ^
12:32.63   explicit
12:32.63 1 warning generated.
12:32.88 4 warnings generated.
12:33.20 toolkit/xre/dllservices
12:33.63 In file included from Unified_cpp_components_viaduct0.cpp:20:
12:33.63 M:/src/toolkit/components/viaduct/fetch_msg_types.pb.cc(197,32): error: bad implicit conversion constructor for 'Request_HeadersEntry_DoNotUse'
12:33.63 Request_HeadersEntry_DoNotUse::Request_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena)
12:33.64                                ^
12:33.64 M:/src/toolkit/components/viaduct/fetch_msg_types.pb.cc(197,32): note: consider adding the explicit keyword to the constructor
12:33.64 Request_HeadersEntry_DoNotUse::Request_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena)
12:33.64                                ^
12:33.64                                explicit
12:33.64 M:/src/toolkit/components/viaduct/fetch_msg_types.pb.cc(691,33): error: bad implicit conversion constructor for 'Response_HeadersEntry_DoNotUse'
12:33.64 Response_HeadersEntry_DoNotUse::Response_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena)
12:33.64                                 ^
12:33.64 M:/src/toolkit/components/viaduct/fetch_msg_types.pb.cc(691,33): note: consider adding the explicit keyword to the constructor
12:33.64 Response_HeadersEntry_DoNotUse::Response_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena)
12:33.64                                 ^
12:33.64                                 explicit
12:33.64 4 errors generated.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #5)

Hmm, I got this error with --enable-warnings-as-errors on Windows.

Oh? --disable-warnings-as-errors does not fix the error.

Yes, explicit should be added in fetch_msg_types.pb.h at like 129 and 443:
explicit Request_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena);
explicit Response_HeadersEntry_DoNotUse(::PROTOBUF_NAMESPACE_ID::Arena* arena);

It's very odd that this isn't failing in CI because we do have builds with the clang plugin enabled by default. Also, it's weird that explicit got removed from those two lines in the first place because fetch_msg_types.pb.h was regenerated using the same protoc compiler version as before (3.11.4, same as our vendored copy of protobuf).

Regardless, I've been looking at updating our in-tree protobuf to a newer version in bug 1773604 and the newer protoc releases appear to preserve explicit on those lines when regenerating classes, so I'm thinking it would probably be fine to re-add them for now as a temporary fix.

Masayuki, I'm curious what's different about your build config that's causing that bustage to be hit while not being hit in CI. Can you share your mozconfig? And I'm assuming you've bootstrapped recently?

Flags: needinfo?(masayuki)

It's not clear why they were removed in the first place, but it breaks
the build for some people and newer protobuf/protoc releases add them
back anyway.

We use ac_add_options --enable-clang-plugin and ac_add_options --enable-warnings-as-errors; the last bootstrap was a while ago. And MozillaBuildSetup is also still not the latest.

Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50740f62bd3d
Add explicit keyword back to lines where it was removed during regeneration. r=markh

My mozconfig is:

export CC="clang-cl.exe"
export CXX="clang-cl.exe"
export LINKER="lld-link.exe"

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../fx64-dbg
mk_add_options MOZ_MAKE_FLAGS="-j32"
mk_add_options AUTOCLOBBER=1

ac_add_options --target=x86_64-pc-mingw32

ac_add_options --enable-rust-simd
ac_add_options --enable-debug
ac_add_options --enable-clang-plugin
ac_add_options --enable-warnings-as-errors

Neither commenting out --enable-rust-simd nor explicitly disabling warnings-as-errors helps. I use MozillaBuild 4.0.1, and I ran ./mach bootstrap for desktop firefox (2) and rustup update manually for the bustage, but they also didn't help to fix this.

Flags: needinfo?(masayuki)

which clang-cl.exe returns ~/.mozbuild/clang/bin/clang-cl.exe, and which lld-link.exe returns ~/.mozbuild/clang/bin/lld-link.exe in the terminal of MozillaBuild.

What does clang-cl --version say?

$ clang-cl --version
clang version 14.0.5 (taskcluster-cdUxRyPsTDCqEgl650LZzQ)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: C:\Users\toybox\.mozbuild\clang\bin

Oh, should I change the target in mozconfig?

Ah, no setting it to --target=x86_64-pc-windows-msvc causes error:

checking for host system type...
x86_64-pc-mingw32
checking for target system type...
Executing: `sh M:/src/build/moz.configure/../autoconf/config.sub x86_64-pc-windows-msvc`
The command returned non-zero exit status 1.
Its error output was:
| Invalid configuration `x86_64-pc-windows-msvc': OS `msvc' not recognized
Command `sh M:/src/build/moz.configure/../autoconf/config.sub x86_64-pc-windows-msvc` failed with exit status 1.

And according to the log of tryserver, CI uses:

[task 2022-06-22T22:25:45.559Z] 22:25:45     INFO -  Adding configure options from /builds/worker/checkouts/gecko/.mozconfig
[task 2022-06-22T22:25:45.559Z] 22:25:45     INFO -    --enable-crashreporter
[task 2022-06-22T22:25:45.559Z] 22:25:45     INFO -    --enable-js-shell
[task 2022-06-22T22:25:45.559Z] 22:25:45     INFO -    --enable-rust-simd
[task 2022-06-22T22:25:45.560Z] 22:25:45     INFO -    --target=x86_64-pc-mingw32
[task 2022-06-22T22:25:45.560Z] 22:25:45     INFO -    --enable-debug
[task 2022-06-22T22:25:45.560Z] 22:25:45     INFO -    --with-branding=browser/branding/nightly

As per discussion on Matrix, it's likely a bug in the handling of ThirdPartyPaths.txt in the plugin on Windows.

Masayuki, can you try this?

diff --git a/build/clang-plugin/Utils.h b/build/clang-plugin/Utils.h
index c25caf43eeffb..dbff493d39b8b 100644
--- a/build/clang-plugin/Utils.h
+++ b/build/clang-plugin/Utils.h
@@ -404,7 +404,8 @@ inline bool inThirdPartyPath(SourceLocation Loc, const SourceManager &SM) {
     auto PathB = sys::path::begin(FileName);
     auto PathE = sys::path::end(FileName);
 
-    auto ThirdPartyB = sys::path::begin(MOZ_THIRD_PARTY_PATHS[i]);
+    auto ThirdPartyB = sys::path::begin(MOZ_THIRD_PARTY_PATHS[i],
+                                        sys::path::Style::windows_slash);
     auto ThirdPartyE = sys::path::end(MOZ_THIRD_PARTY_PATHS[i]);
 
     for (; PathB != PathE; ++PathB) {
Flags: needinfo?(masayuki)

I got this compile errors...

 0:14.66 In file included from M:/src/build/clang-plugin/MemMoveAnnotation.h:10:
 0:14.66 M:/src/build/clang-plugin/Utils.h(410,39): error: too many arguments to function call, expected single argument 'path', have 2 arguments       
 0:14.66                                       sys::path::Style::windows_slash);
 0:14.66                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:14.66 C:/Users/toybox/.mozbuild/clang/include\llvm/Support/Path.h(128,16): note: 'end' declared here
 0:14.67 const_iterator end(StringRef path);
 0:14.67                ^
 0:14.67 In file included from M:/src/build/clang-plugin/MustReturnFromCallerChecker.cpp:5:
 0:14.67 In file included from M:/src/build/clang-plugin/MustReturnFromCallerChecker.h:8:
 0:14.67 In file included from M:/src/build/clang-plugin/RecurseGuard.h:8:
 0:14.67 M:/src/build/clang-plugin/Utils.h(410,39): error: too many arguments to function call, expected single argument 'path', have 2 arguments       
 0:14.67                                       sys::path::Style::windows_slash);
 0:14.67                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:14.67 C:/Users/toybox/.mozbuild/clang/include\llvm/Support/Path.h(128,16): note: 'end' declared here
 0:14.67 const_iterator end(StringRef path);
Flags: needinfo?(masayuki)

I updated the patch in comment 20.

Flags: needinfo?(masayuki)

I applied the patch to 4e41d182f1f6 which is before landing the fix. However, I still see the same errors.

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

Attachment

General

Creator:
Created:
Updated:
Size: