Skip to content
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

Workarounds for Building with Clang on Ubuntu 22 #3008

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

upshaw-alex
Copy link
Contributor

@upshaw-alex upshaw-alex commented May 5, 2023

This PR introduces workarounds for building Concord-BFT with Clang after the Ubuntu 22 upgrade.

These workarounds include:

  • Removal of the local variable totalVal from the function libutt::Factory::randomWallets, as this variable is unused and (with -Werror) Clang generates an error for that.
  • Changing usage of std::optional<::rocksdb::PinnableSlice> to std::unique_ptr<::rocksdb::PinnableSlice>, as Clang generates an error with this particular template instantiation. Some additional context on this one: (1) std::optional<::rocksdb::PinnableSlice> is used in Concord-BFT only as a return type for concord::storage::rocksdb::NativeClient::getSlice; (2) the particular error I saw is /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/optional:158:7: error: the parameter for this explicitly-defaulted copy assignment operator is const, but a member or base requires it to be non-const operator=(const _Optional_payload_base&) = default;. Of possible interest for this error is that I think ::rocksdb::PinnableSlice is not copy constructable, though this documentation suggests std::optional's copy constructor should be defined as deleted if the base type is not copy constructable, so I am still not entirely sure about the nature of this error; (3) I am aware this type change may be a pessimization with respect to memory management (moving objects from the stack to the heap); (4) I chose this over other possible workarounds I thought of as it required fewer code changes.
  • Problem Overview
    Building Concord-BFT with Clang (specifically Clang 14) seems to be broken as of the transition to Ubuntu 22; this is a problem for consumers of Concord-BFT that currently rely on Clang-specific instrumentation.
  • Testing Done
    I have confirmed a Concord-BFT-dependent project I have been involved in builds with the fixes I have here, using Clang on Ubuntu 22. I am hoping Concord-BFT's GitHub CI will show whether these changes are breaking to Concord-BFT's default build settings.

@upshaw-alex upshaw-alex requested a review from a team as a code owner May 5, 2023 22:24
@upshaw-alex upshaw-alex force-pushed the fixes-for-clang-on-ubuntu-22 branch from 6cd292f to 59cce5f Compare May 8, 2023 18:55
@upshaw-alex
Copy link
Contributor Author

I have force-pushed a rebase of my commit for this PR off of latest master in the interest of keeping it up-to-date.

This commit introduces workarounds for building Concord-BFT with Clang
after the Ubuntu 22 upgrade.

These workarounds include:
- Changing usage of std::optional<::rocksdb::PinnableSlice> to
  std::unique_ptr<::rocksdb::PinnableSlice>, as Clang generates an error
  with this particular template instantiation.
- Removal of the local variable totalVal from the function
  libutt::Factory::randomWallets, as this variable is unused and (with
  -Werror) Clang generates an error for that.
@upshaw-alex upshaw-alex force-pushed the fixes-for-clang-on-ubuntu-22 branch from 59cce5f to a09ef6d Compare May 9, 2023 17:08
@upshaw-alex
Copy link
Contributor Author

I have force-pushed a rebase of my commit for this PR off of latest master in the interest of keeping it up to date.

Copy link

@rachitchadha rachitchadha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks okay to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants