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 system tweaks patchset #618

Merged

Conversation

ChrisCummins
Copy link
Contributor

@ChrisCummins ChrisCummins commented Mar 9, 2022

This pull request makes two important changes to the build:

And a bunch of smaller tweaks to the CMake build:

  • Switches the CI cmake job to use a pre-compiled LLVM.
  • Updates the version of absl and benchmark to work around some build errors on newer linux releases.
  • Updates ProGraML so that it uses the same version of absl.
  • Turns off the redundant conditional logic for building on Darwin. There is no Linux/macOS-dependent logic in the build system (except for janky linker workarounds needed by bazel).
  • Automatically enables ccache in the CMake config when it is available.
  • Automatically uses the lld linker when it is available.
  • Forces the Ninja generator.
  • Makes a number of small formatting tweaks for readability.

My eventual goal is to add support for CMake on macOS (#618), but I am hitting a problem with the protobuf configuration step (autoreconf) in external/protobuf/build_protobuf.cmake:

checking for g++ options needed to detect all undeclared functions... cannot detect
configure: error: in `/Users/cummins/src/CompilerGym/build/external/protobuf/protobuf/src/protobuf':
configure: error: cannot make g++ report undeclared builtins
See `config.log' for more details
CMake Error at /Users/cummins/src/CompilerGym/external/protobuf/build_protobuf.cmake:50 (execute_process):
  execute_process failed command indexes:

    1: "Child return code: 1"

For some reason, the configure script doesn't like the detected C/C++ compiler. I'll pick this up at a later date.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 9, 2022
@ChrisCummins ChrisCummins changed the title [cmake] Enable building on macOS [cmake] Enable building on macOS (& a handful of other tweaks) Mar 9, 2022
@ChrisCummins ChrisCummins force-pushed the feature/cmake-macos branch 2 times, most recently from d56653d to 43e6dfc Compare March 10, 2022 16:32
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #618 (26e287d) into development (4ca7f19) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #618      +/-   ##
===============================================
- Coverage        88.60%   88.54%   -0.06%     
===============================================
  Files              115      115              
  Lines             7019     7019              
===============================================
- Hits              6219     6215       -4     
- Misses             800      804       +4     
Impacted Files Coverage Δ
...er_gym/third_party/inst2vec/inst2vec_preprocess.py 69.73% <0.00%> (-4.22%) ⬇️
compiler_gym/util/commands.py 83.33% <0.00%> (-2.39%) ⬇️
compiler_gym/envs/compiler_env.py 92.34% <0.00%> (+0.49%) ⬆️
compiler_gym/views/observation.py 100.00% <0.00%> (+2.70%) ⬆️
...loop_tool/service/loop_tool_compilation_session.py 90.47% <0.00%> (+2.72%) ⬆️
compiler_gym/views/observation_space_spec.py 89.28% <0.00%> (+3.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ca7f19...26e287d. Read the comment docs.

@ChrisCummins ChrisCummins changed the title [cmake] Enable building on macOS (& a handful of other tweaks) Build system tweak patchset Mar 10, 2022
@ChrisCummins
Copy link
Contributor Author

@sogartar could you take a quick look at this?

Cheers,
Chris

@ChrisCummins ChrisCummins changed the title Build system tweak patchset Build system tweaks patchset Mar 10, 2022
@ChrisCummins
Copy link
Contributor Author

Merging now as this fixes the development build.

Cheers,
Chris

@ChrisCummins ChrisCummins merged commit 549497a into facebookresearch:development Mar 11, 2022
@ChrisCummins ChrisCummins deleted the feature/cmake-macos branch March 11, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants