Add Building with CMake #498
Conversation
facebook-github-bot
added
the
CLA Signed
|
I would like to have the CI run for this PR. |
|
Looking good @sogartar! I'll have a more detailed pick through the changes and run the instructions from the updated README when I find time, probably towards the end of this week. :-)
Done! Cheers, |
Codecov Report
@@ Coverage Diff @@
## development #498 +/- ##
===============================================
- Coverage 87.37% 87.08% -0.30%
===============================================
Files 113 113
Lines 6401 6403 +2
===============================================
- Hits 5593 5576 -17
- Misses 808 827 +19
Continue to review full report at Codecov.
|
|
Ah, I just noticed that this is marked WIP. I'll hold back on taking a look until you request a review 🙂 Cheers, |
sogartar
force-pushed
the
boian-cmake-2
branch
from
1f94358
to
57b490e
Compare
|
@ChrisCummins, will it be possible to have the CI run automatically when I update the PR branch? |
|
Hmm that's strange, normally I only have to approve the CI running once per new contributor - looks like it disabled itself somehow! :-) The CI should be active again now, and will automatically re-run every time you push to the branch. Cheers, |
|
@ChrisCummins, I may be considered a first time contributor, because my contribution technically has not been approved yet. |
|
Ah, that must be it! I don't have access to the settings page to change that behavior, so if you would like to file a meaningless one line PR then I can merge that to give you contributor status :) Cheers, |
|
@ChrisCummins, here is my empty PR #499. |
|
@ChrisCummins, I think the PR is in a good shape for a review. |
|
Great thanks @sogartar, I will take a look tomorrow! Yep, I'm happy to split things over multiple PRs to make reviewing easier. Cheers, |
There was a problem hiding this comment.
Hi @sogartar, I have a few other commitments ATM so it may take me a couple of passes to go through all of this PR, but I thought it'd be worth sharing my notes so far.
I followed along with your new INSTALL.md steps on this machine:
$ cat /etc/os-release
NAME="Pop!_OS"
VERSION="20.10"
ID=pop
ID_LIKE="ubuntu debian"
PRETTY_NAME="Pop!_OS 20.10"
VERSION_ID="20.10"
HOME_URL="https://pop.system76.com"
SUPPORT_URL="https://support.system76.com"
BUG_REPORT_URL="https://github.com/pop-os/pop/issues"
PRIVACY_POLICY_URL="https://system76.com/privacy"
VERSION_CODENAME=groovy
UBUNTU_CODENAME=groovy
LOGO=distributor-logo-pop-os
I was able to get as far as running the actual build, when I hit an error that I'm not sure how to work around:
$ cmake --build "$BUILD_DIR"
[1532/1767] Generating csmith
FAILED: compiler_gym/third_party/csmith/csmith
cd /home/cec/src/CompilerGym/cmake_build/compiler_gym/third_party/csmith && /home/cec/.local/bin/cmake -E create_symlink /home/cec/src/CompilerGym/cmake_build/compiler_gym/third_party/csmith/csmith/bin/csmith /home/cec/src/CompilerGym/cmake_build/compiler_gym/third_party/csmith/csmith
failed to create symbolic link '/home/cec/src/CompilerGym/cmake_build/compiler_gym/third_party/csmith/csmith' because existing path cannot be removed: Is a directory
[1557/1767] Building C object _deps/grpc-build/th...akeFiles/crypto.dir/src/crypto/fipsmodule/bcm.c.o
ninja: build stopped: subcommand failed.
cmake --build "$BUILD_DIR" 1023.32s user 116.98s system 1955% cpu 58.314 total
Also, it looks like the cmake build on the CI took 4.5hrs, is this to be expected?
On my machine, the initial cmake invocation took 31 min, then about 1 minute of actual build before I hit the error posted above.
I left some other notes inline :) Hope they're helpful!
Cheers,
Chris
| -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \ # For faster rebuilds, can be removed | ||
| -DCMAKE_EXE_LINKER_FLAGS_INIT="-fuse-ld=lld" -DCMAKE_MODULE_LINKER_FLAGS_INIT="-fuse-ld=lld" -DCMAKE_SHARED_LINKER_FLAGS_INIT="-fuse-ld=lld" \ # For faster builds, can be removed |
There was a problem hiding this comment.
| -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \ # For faster rebuilds, can be removed | |
| -DCMAKE_EXE_LINKER_FLAGS_INIT="-fuse-ld=lld" -DCMAKE_MODULE_LINKER_FLAGS_INIT="-fuse-ld=lld" -DCMAKE_SHARED_LINKER_FLAGS_INIT="-fuse-ld=lld" \ # For faster builds, can be removed | |
| -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \ | |
| -DCMAKE_EXE_LINKER_FLAGS_INIT="-fuse-ld=lld" -DCMAKE_MODULE_LINKER_FLAGS_INIT="-fuse-ld=lld" -DCMAKE_SHARED_LINKER_FLAGS_INIT="-fuse-ld=lld" \ |
On my shell (zsh 5.8), anything after the escaping \ is invalid and causes a syntax error:
$ cmake -GNinja \
-DCMAKE_C_COMPILER=clang-9 \
-DCMAKE_CXX_COMPILER=clang++-9 \
-DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
-DCMAKE_EXE_LINKER_FLAGS_INIT="-fuse-ld=lld"-DCMAKE_MODULE_LINKER_FLAGS_INIT="-fuse-ld=lld" -DCMAKE_SHARED_LINKER_FLAGS_INIT="-fuse-ld=lld" \
-DPython3_FIND_VIRTUALENV=FIRST \
-S "$SOURCE_DIR" \
-B "$BUILD_DIR"
zsh: no matches found: #
zsh: no matches found: #
zsh: command not found: -DPython3_FIND_VIRTUALENV=FIRST
There was a problem hiding this comment.
I reorganized the instructions and this problem is not present there.
There was a problem hiding this comment.
Looks like this optional arguments are still in the base command though. Tbh I think it would be fine to simply update the lld to lld-9 and then include them as part of the base command, since the lld-9 package is part of the required dependencies.
There was a problem hiding this comment.
I updated to use lld-9 explicitly. I think on your system, lld-9 is not the default lld.
INSTALL.md
Outdated
| wget -O - https://apt.kitware.com/keys/kitware-archive-latest.asc 2>/dev/null | gpg --dearmor - | sudo tee /etc/apt/trusted.gpg.d/kitware.gpg >/dev/null | ||
| sudo apt-add-repository "deb https://apt.kitware.com/ubuntu/ $(lsb_release -cs) main" | ||
| sudo apt update | ||
| sudo apt install kitware-archive-keyring | ||
| sudo rm /etc/apt/trusted.gpg.d/kitware.gpg | ||
| sudo apt update | ||
| sudo apt install cmake=3.20\* cmake-data=3.20\* | ||
| ``` |
There was a problem hiding this comment.
This didn't work for me:
$ sudo apt-add-repository -y "deb https://apt.kitware.com/ubuntu/ $(lsb_release -cs) main"
Repository: 'deb https://apt.kitware.com/ubuntu/ groovy main'
Description:
Archive for codename: groovy components: main
More info: https://apt.kitware.com/ubuntu/
Adding repository.
Adding deb entry to /etc/apt/sources.list.d/archive_uri-https_apt_kitware_com_ubuntu_-groovy.list
Adding disabled deb-src entry to /etc/apt/sources.list.d/archive_uri-https_apt_kitware_com_ubuntu_-groovy.list
Hit:1 http://ppa.launchpad.net/jgmath2000/et/ubuntu groovy InRelease
Get:2 https://download.docker.com/linux/ubuntu groovy InRelease [43.3 kB]
Hit:3 http://ppa.launchpad.net/system76/pop/ubuntu groovy InRelease
Hit:4 http://dl.google.com/linux/chrome/deb stable InRelease
Hit:5 http://us.archive.ubuntu.com/ubuntu groovy InRelease
Hit:6 http://us.archive.ubuntu.com/ubuntu groovy-security InRelease
Hit:7 http://apt.pop-os.org/proprietary groovy InRelease
Hit:8 http://us.archive.ubuntu.com/ubuntu groovy-updates InRelease
Hit:9 https://download.sublimetext.com apt/stable/ InRelease
Hit:10 http://us.archive.ubuntu.com/ubuntu groovy-backports InRelease
Ign:11 https://apt.kitware.com/ubuntu groovy InRelease
Err:12 https://apt.kitware.com/ubuntu groovy Release
404 Not Found [IP: 66.194.253.25 443]
Reading package lists... Done
E: The repository 'https://apt.kitware.com/ubuntu groovy Release' does not have a Release file.
N: Updating from such a repository can't be done securely, and is therefore disabled by default.
N: See apt-secure(8) manpage for repository creation and user configuration details.
Rather than installing through the package manager, how about using a specific binary? This is what I do for hadolint/bazel on Linux. Something like:
wget https://github.com/Kitware/CMake/releases/download/v3.20.5/cmake-3.20.5-linux-x86_64.sh -O cmake.sh
bash cmake.sh --prefix=$HOME/.local --exclude-subdir --skip-license
rm cmake.shThat will install to ~/.local/bin/cmake.
There was a problem hiding this comment.
I also had to remove the -fuse-ld flags on my machine:
$ ~/.local/bin/cmake -GNinja -DCMAKE_C_COMPILER=clang-9 -DCMAKE_CXX_COMPILER=clang++-9 -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_EXE_LINKER_FLAGS_INIT="-fuse-ld=lld"-DCMAKE_MODULE_LINKER_FLAGS_INIT="-fuse-ld=lld" -DCMAKE_SHARED_LINKER_FLAGS_INIT="-fuse-ld=lld" -DPython3_FIND_VIRTUALENV=FIRST -S "$SOURCE_DIR" -B "$BUILD_DIR"
-- Check for working C compiler: /bin/clang-9 - broken
CMake Error at /home/cec/.local/share/cmake-3.20/Modules/CMakeTestCCompiler.cmake:66 (message):
The C compiler
"/bin/clang-9"
is not able to compile a simple test program.
It fails with the following output:
Change Dir: /home/cec/src/CompilerGym/cmake_build/CMakeFiles/CMakeTmp
Run Build Command(s):/bin/ninja cmTC_0c2f4 && [1/2] Building C object CMakeFiles/cmTC_0c2f4.dir/testCCompiler.c.o
[2/2] Linking C executable cmTC_0c2f4
FAILED: cmTC_0c2f4
: && /bin/clang-9 -fuse-ld=lld-DCMAKE_MODULE_LINKER_FLAGS_INIT=-fuse-ld=lld CMakeFiles/cmTC_0c2f4.dir/testCCompiler.c.o -o cmTC_0c2f4 && :
clang: error: invalid linker name in argument '-fuse-ld=lld-DCMAKE_MODULE_LINKER_FLAGS_INIT=-fuse-ld=lld'
ninja: build stopped: subcommand failed.
Diff between the command that failed and the command that worked:
- $ ~/.local/bin/cmake -GNinja -DCMAKE_C_COMPILER=clang-9 -DCMAKE_CXX_COMPILER=clang++-9 -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_EXE_LINKER_FLAGS_INIT="-fuse-ld=lld"-DCMAKE_MODULE_LINKER_FLAGS_INIT="-fuse-ld=lld" -DCMAKE_SHARED_LINKER_FLAGS_INIT="-fuse-ld=lld" -DPython3_FIND_VIRTUALENV=FIRST -S "$SOURCE_DIR" -B "$BUILD_DIR"
+ $ ~/.local/bin/cmake -GNinja -DCMAKE_C_COMPILER=clang-9 -DCMAKE_CXX_COMPILER=clang++-9 -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DPython3_FIND_VIRTUALENV=FIRST -S "$SOURCE_DIR" -B "$BUILD_DIR"There was a problem hiding this comment.
Did you install the lld-9 package? It is part of one of the steps. Supposedly, lld is a faster linker? I wanted to use it to speed up the build. Some parts of LLVM take a long time to link.
There was a problem hiding this comment.
Yep I have lld-9, although on my system it only installs the versioned binary, so maybe the problem is its looking for an unversioned lld binary?
$ which lld-9
/bin/lld-9
$ which lld
lld not foundThere was a problem hiding this comment.
Just to confirm, using lld-9 worked for me:
~/.local/bin/cmake -GNinja -DCMAKE_C_COMPILER=clang-9 -DCMAKE_CXX_COMPILER=clang++-9 -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_EXE_LINKER_FLAGS_INIT="-fuse-ld=lld-9" -DCMAKE_MODULE_LINKER_FLAGS_INIT="-fuse-ld=lld-9" -DCMAKE_SHARED_LINKER_FLAGS_INIT="-fuse-ld=lld-9" -DPython3_FIND_VIRTUALENV=FIRST -S "$SOURCE_DIR" -B "$BUILD_DIR"
INSTALL.md
Outdated
| ### Dependency instructions for Ubuntu | ||
|
|
||
| #### CMake | ||
| Requires CMake (==3.20). |
There was a problem hiding this comment.
Does it have to be this specific version? Could be relaxed to older/newer versions? The conda version of cmake lags behind at 3.19, but would make for a much easier install (conda install -c anaconda cmake)
There was a problem hiding this comment.
I used some 3.20 stuff and I had problems with a later version when downloading the external dependencies.
| @@ -0,0 +1,48 @@ | |||
| #[=======================================================================[.rst: | |||
There was a problem hiding this comment.
For this file (and all other *.cmake files), is it your own code? If so it needs the CompilerGym license header. If it's taken from another project, does it need a license header + attribution for that project?
There was a problem hiding this comment.
Some parts of the code come from IREE's Bazel to CMake autoconversion tool. That is under Apache 2.0 license and it needs attribution.
There was a problem hiding this comment.
Okay, in that case I think that the *.cmake files should have IREE's license header added
I think that is caused because the source directory and the binary directory are the same. I put an error message for that.
This is a super build that builds most dependencies with Compiler Gym. The configuration step takes so long because many of the dependencies are built then. They need to be found after being built, which is a part of the configuration and not the build. CMake's find_package works during configuration. |
It turned out that I did not specify ninja as generator in the CI script, so make was used by default and by default it runs one job at a time. I fixed it, it should be faster now. |
|
Hi @sogartar, thanks for quickly addressing those comments!
I had the build directory as a subdir of the source directory. Not sure if that matters. Either way, I have pulled your latest changes and am re-running the cmake commands, will let you know how I get on :) Cheers, |
|
I initially hit an error with a missing python module: After installing it using: I was able to complete the build! Nice :) Cheers, |
|
This is included in the |
|
Hi @sogartar,
Ah sorry, I missed that you had changed requirements.txt. I might split it into a separate file since it is a build-only dependency and It looks like the build is nearly ready to land :-) The only failure I see is the I will take a look through the rest of the PR today. Cheers, |
.github/workflows/ci.yaml
Outdated
| - name: Persist the cmake build | ||
| uses: actions/cache@v2 | ||
| id: cmake_build_cache | ||
| with: | ||
| path: ~/cmake_build | ||
| key: cmake_build-${{ runner.os }}-${{ github.sha }} |
There was a problem hiding this comment.
Nice! Are there any times when this build directory needs to be cleaned? I don't know much about cmake but I recall sometimes having to rm the build directory after changing the build config
There was a problem hiding this comment.
I reorganized the CI jobs and removed the cache, because I could not make it be shared between jobs. Now the building and testing are part of the same job. It may be due to the fact that the build dir is huge. On my system when building in release with debug info it is 70+ GiB.
There was a problem hiding this comment.
Sounds good to me. I use the artifact action to share the built wheels between the various test jobs, but I guess a 70+ GB artifact wouldn't work :)
|
I went through the rest of the PR and left another bunch of comments. Those, along with fixing/disabling the CI test job, and I think it'll be ready to land :) Great work @sogartar! Cheers, |
I addressed your comments. Let me know if it looks good, so I can squash and rebase before merging. |
|
Hi @sogartar, thanks for addressing all those comments. LGTM! Please squash and rebase and I'll get it merged :) Cheers, |
sogartar
force-pushed
the
boian-cmake-2
branch
from
d750768
to
3a8f806
Compare
|
@ChrisCummins I rebased onto the https://github.com/facebookresearch/CompilerGym/runs/4486563513?check_suite_focus=true#step:5:27193 It looks like that ProGraML uses an older version of protobuf, namely 3.13.0. It includes the generated protobuf header in its interface. I don't know if it is intentional. If it is intentional the protobuf versions in Compiler Gym and ProGraML must match. I don't know why this problem does not appear in Bazel. Does it somehow resolve to using only one version of the proto family of functions and by extension the same protobuf version? |
sogartar
force-pushed
the
boian-cmake-2
branch
from
3a8f806
to
a3c2094
Compare
Co-authored-by: kyle <kyle@nod-labs.com>
sogartar
force-pushed
the
boian-cmake-2
branch
from
a3c2094
to
5e3516e
Compare
|
I reverted the version of protobuf in CMake. It is now different from that in Bazel. It should be advanced at a later stage. |
|
Hi @sogartar, many thanks for the detailed description of the problem, and I agree with your decision to pin the version of protobuf in CMake to match ProGraML.
I'm afraid I don't know either, but I suspect you're right. I think when we declare the dependency in the CompilerGym WORKSPACE here: https://github.com/facebookresearch/CompilerGym/blob/development/WORKSPACE#L103-L111 that then makes all references to the At some point I'll update the ProGraML dependency to match the version of proto used in CompilerGym. Cheers, |
This is a WIP of the CMake building.
This PR supersede #478.
Currently the building of the
compiler_gymdirectory is done.Building of tests, benchmarks, examples and the python package itself lies ahead.
There is a bit of cleanup and renaming to do after that. Some of the functionality form the auto-conversion from Bazel to CMake may prove to be unnecessary.
I plan not to use CMake's ctest functionality for tests, since it is not a part of the building itself. I will make the tests and benchmarks proper build targets.