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

Build improvements #4841

Merged
merged 4 commits into from
Oct 6, 2023
Merged

Build improvements #4841

merged 4 commits into from
Oct 6, 2023

Conversation

Thelta
Copy link
Contributor

@Thelta Thelta commented Sep 17, 2023

Setting up Orbit on ArchLinux wasn't easy, but I made some tweaks to make it work better. I also wanted to journal my fixes.

  1. Added boost version header to third_party/Outcome/outcome.hpp. Before that, because header was not included, BOOST_VERSION was evaluated to 0 and was using wrong definitions which caused build errors. Probably cause of Doesn't build with Boost outcome #4822.

  2. For my compiler (gcc 13.2), libprocinfo was giving ignored attributes warning, which caused error because of Wall. Passed Wno-error for this warning to libprocinfo target.

  3. In ArchLinux, LLVM libraries is distributed via a singular shared library libLLVM.so. Search this library in found LLVM's library path, if found use it, if not use the static libraries.

  4. Currently Orbit uses grpc, protobuf and abseil, however only abseil is forced to be built if conan is not used. This causes errors because in ArchLinux distribution of protobuf uses much newer abseil than Orbit's abseil and this causes errors. Currently finding if abseil is built with C++17 or not is not possible, so warn the users about it and how to circumvent it.

With these patches only one build error remains, current ArchLinux distribution of protobuf is not compatible this repo's libprotobuf-mutator. However this would required adding patches to a third party library, which I am wary. So I decided to add this patch to not here but my own Orbit repo.

@google-cla
Copy link

google-cla bot commented Sep 17, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@pierricgimmig
Copy link
Collaborator

@Thelta, thanks a lot for the PR. This looks good to me, but perhaps @beckerhe can also have a look. I kicked off the CI, let's see if everything still builds fine on Ubuntu and Windows.

Copy link
Collaborator

@beckerhe beckerhe left a comment

Choose a reason for hiding this comment

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

Thank you for the fixes and sorry for the delay. According to the CI there is an issue with the LLVM change

Comment on lines 7 to 17
find_library(LLVM_LIBS LLVM NO_SYSTEM_ENVIRONMENT_PATH NO_CMAKE_SYSTEM_PATH PATHS ${LLVM_LIBRARY_DIR})

if(NOT LLVM_LIBS)
set(LLVM_LIBS
LLVMDebugInfoCodeView
LLVMDebugInfoDWARF
LLVMDebugInfoPDB
LLVMHeaders
LLVMObject
LLVMSymbolize)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the CI results now. I don't think this works. What exactly would you like to achieve here?

Copy link
Contributor Author

@Thelta Thelta Sep 21, 2023

Choose a reason for hiding this comment

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

First it tries to find libLLVM.so (this shared library includes everything and some distributions only includes this library). If not then tries to use the static libraries (LLVMDebugInfoCodeView, LLVMDebugInfoDWARF etc).

However I made two mistakes.

  1. I assumed distributions either released only shared library (libLLVM.so) or static libraries. However in ubuntu's case (and probably others) they also release the shared library. This resulted LLVM_LIBS only using libLLVM.so

  2. Because of above, LLVMHeaders, which is not a static library but a interface target which includes LLVM headers, are not included. This causes the build errors.

9c3160 should fix both issues. With this patch, it first searches the necessary llvm static libraries, if one is not found, then uses the shared library. I also readded LLVMHeaders to target_link_libraries directly.

@beckerhe beckerhe self-requested a review September 21, 2023 05:51
@Thelta Thelta force-pushed the build-improvements branch 2 times, most recently from dbbec0d to 4e8a45d Compare September 21, 2023 16:33
@pierricgimmig
Copy link
Collaborator

@Thelta, could you rebase on latest main, the Windows build is fixed now with #4851. Thanks!

@Thelta Thelta force-pushed the build-improvements branch from 554d20c to 0cff9d5 Compare October 5, 2023 11:09
Thelta added 2 commits October 5, 2023 14:11
- Because BOOST_VERSION isn't available it is evaluated as 0.
  This causes error's for boost >= 1.76
- At least in gcc 13.2, process.cpp:81 gives ignored attributes
  warning.
@Thelta Thelta force-pushed the build-improvements branch from 0cff9d5 to 84c2322 Compare October 5, 2023 11:12
Copy link
Collaborator

@pierricgimmig pierricgimmig left a comment

Choose a reason for hiding this comment

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

lgtm

@pierricgimmig
Copy link
Collaborator

Looks like the Windows build is failing:

CMake Warning at cmake/FindLLVM.cmake:13 (find_package):
  Could not find a configuration file for package "LLVM" that is compatible
  with requested version "14".

Thelta added 2 commits October 6, 2023 11:57
- In some distributions of LLVM, like in ArchLinux, static libraries
  are not included.
@Thelta Thelta force-pushed the build-improvements branch from 84c2322 to 7bba401 Compare October 6, 2023 09:08
@Thelta
Copy link
Contributor Author

Thelta commented Oct 6, 2023

Looks like the Windows build is failing:

CMake Warning at cmake/FindLLVM.cmake:13 (find_package):
  Could not find a configuration file for package "LLVM" that is compatible
  with requested version "14".

Yeah it seems unlike vcpkg, conan directly loads dependencies as targets. New rebase should fix it.

@pierricgimmig pierricgimmig merged commit e7d98f6 into google:main Oct 6, 2023
11 checks passed
@pierricgimmig
Copy link
Collaborator

Thanks for the change, @Thelta !

@Thelta Thelta deleted the build-improvements branch October 14, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants