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

Some small cmake tweaks #666

Merged

Conversation

ChrisCummins
Copy link
Contributor

Some small tweaks to attempt to reduce the number of flags needed to pass to cmake for most use cases:

  • Python3_FIND_VIRTUALENV=FIRST is the default value, so don't specify it in install instructions.
  • Define CMAKE_BUILD_WITH_INSTALL_RPATH in CMakeLists.txt rather than from command line.
  • Don't override command line flags to set ccache / lld paths.

@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 Apr 27, 2022
@sogartar
Copy link

sogartar commented Apr 27, 2022

I don't think that the CompilerGym Python package utilizes rpath. It should also be installable anywhere. We don't install through CMake's install target. CMAKE_BUILD_WITH_INSTALL_RPATH will hardcode the rpath based on CMAKE_INSTALL_PREFIX.

@ChrisCummins
Copy link
Contributor Author

Is it a good idea to hardcode it as I have done here?

@ChrisCummins
Copy link
Contributor Author

Deferring to you on this @sogartar as the cmake expert 🙂

@sogartar
Copy link

sogartar commented Apr 28, 2022

I think we don't need CMAKE_BUILD_WITH_INSTALL_RPATH because the service executables link statically to their dependencies.

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2022

Codecov Report

Merging #666 (e43ad7b) into development (63dbfac) will increase coverage by 0.03%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           development     #666      +/-   ##
===============================================
+ Coverage        88.36%   88.40%   +0.03%     
===============================================
  Files              125      125              
  Lines             7565     7565              
===============================================
+ Hits              6685     6688       +3     
+ Misses             880      877       -3     
Impacted Files Coverage Δ
compiler_gym/envs/llvm/datasets/cbench.py 80.50% <0.00%> (+1.08%) ⬆️

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 63dbfac...e43ad7b. Read the comment docs.

@ChrisCummins ChrisCummins merged commit e9aa1e6 into facebookresearch:development May 12, 2022
@ChrisCummins ChrisCummins deleted the feature/cmake-tweaks branch May 12, 2022 17:32
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.

4 participants