-
Notifications
You must be signed in to change notification settings - Fork 20
Upgrade to Bazel 6, modern protobufs, etc #77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Yes, we should use an older ubuntu for now I guess. Upgrading to a newer LLVM will require some code changes, but yes, will be useful maintenance for us - but ideally as a separate lift! |
I filed https://github.com/reboot-dev/mono/issues/4284 for the llvm-upgrade issue. |
I ran into https://github.com/reboot-dev/mono/issues/4280 yesterday; could your version of Xcode be the issue? |
The issue I ran into is almost the opposite: stout builds fine with the latest XCode, but doesn't build with llvm@14 from brew (+ |
|
What version of OSX are you experiencing this on? It looks like Aside from that, the Mac CI on this PR seems to be failing for yet another reason:
|
f2dd543 to
f442719
Compare
I looked into this. For some reason that is not at all clear to me, on some platforms (e.g., macos-latest CI but not ubuntu-latest CI or my local Mac ARM laptop), Bazel uses I fixed that by adding
That sounds suspiciously like this protobuf issue, which arises because the combination of protobuf's source layout and how Bazel works means it is very easy to exceed the 260 character limit on MSVC filename lengths! It's not exactly clear why adding c++17 to Note that a comment in that GitHub issue helpfully notes:
So if we care about stout on Windows, we probably want to switch to clang instead of MSVC. |
|
@rjhuijsman wdyt about leaving CI on Windows broken for now? As mentioned above, longer-term we probably need to move stout to building with clang on Windows, but that doesn't seem worth my spending time on at the moment. |
b1d6124 to
2aec7db
Compare
|
There's something in the back of my head saying Windows CI was already broken, even? @onelxj would know for sure. Or you can look at what an old PR looks like (I would, but I'm on my phone). I'm ok with keeping Windows CI broken. And bonus points if you can comment out the Windows check before submitting so I don't have to try to remember for next time. 😄 |
|
@rjhuijsman You might be referring to the fact that stout CI on |
|
Got it. But yes, your point stands, not worth your time right now maintaining support for a platform we don't build for. |
d458e4d to
4557a8c
Compare
|
@rjhuijsman Can we land this now? It should be safe, as the submodule SHA in mono should still point at the previous commit. We should be close to landing B6 in mono anyway and we can then update the submodule SHA in mono at that point. |
Yes, we are close! |
4557a8c to
c8c088e
Compare
* Upgrade to Protobuf 29.3 and fix resulting minor breakages to APIs and build rules. This is necessary in order for stout to be used with Bazel projects that depend on newer versions of Protobuf. * Upgrade to recent abseil. This is necessary due to the protobuf upgrade. We take care to use the version of abseil required by gRPC 1.68.2, which is the version that the mono repo will be using once we land the Bazel 6. * Upgrade to Bazel 6. This is necessary because recent abseil does not build with Bazel 5. * Use `proto_library` and `cc_proto_library` from @com_google_protobuf, rather than `rules_proto` and `rules_cc`, respectively. The latter sources are deprecated.
For some reason (and only on some platforms -- in particular, the `macos-latest` GitHub runner), it seems that Bazel decides to use `host_cxxopt` (and not `cxxopt`) when compiling the abseil dependency. This breaks the build, because Abseil requires >= C++14.
c8c088e to
d73790b
Compare
d73790b to
74f3259
Compare
Upgrade to Protobuf 29.3 and fix resulting minor breakages to APIs and build rules. This is necessary in order for stout to be used with Bazel projects that depend on newer versions of Protobuf.
Upgrade to recent abseil. This is necessary due to the protobuf upgrade.
Upgrade to Bazel 6. This is necessary because recent abseil does not build with Bazel 5.
Use
proto_libraryandcc_proto_libraryfrom @com_google_protobuf, rather thanrules_protoandrules_cc, respectively. The latter sources are deprecated.