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

Docker: Add a clang install step to dockerfile #10

Closed
wants to merge 4 commits into from

Conversation

johan-boule
Copy link

These commits add a clang install step to dockerfile.

The script downloads a specific commit hash of the llvm-project git repository (clang being part of it).
That commit was taken from the tip of the master branch as it was yesterday, so it's a pre-release of version 9.

I also changed other build scripts so they determine an appropriate number for job parallelism.

Copy link
Owner

@mathstuf mathstuf left a comment

Choose a reason for hiding this comment

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

Please rebase, reorder, and squash commits for a sequence of atomic commits (can be done later and I can do it as well if I can push to the source branch before merging if you'd prefer).

Dockerfile Outdated
@@ -25,6 +25,9 @@ RUN sh /home/modules/install_cmake.sh
COPY install_ninja.sh /home/modules/install_ninja.sh
RUN sh /home/modules/install_ninja.sh

COPY install_clang.sh /home/modules/install_clang.sh
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned in #3, I think this should be before cmake and ninja so that bumping the "small" projects doesn't require rebuilding clang. We can install a system-wide cmake and ninja-build for that. We should probably also just modify PATH using ENV commands rather than a wrapper script so that it's always available.

Copy link
Author

Choose a reason for hiding this comment

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

OK. Reordered.

cmake -G Ninja \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_INSTALL_PREFIX="$HOME/misc/root/clang" \
-DLLVM_ENABLE_PROJECTS='clang' \
Copy link
Owner

Choose a reason for hiding this comment

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

I think we also want a shared build. That should certainly reduce the installation tree size. I've found that -DLLVM_PARALLEL_LINK_JOBS:STRING=1 was required to not blow memory limits when linking. Adding -DLLVM_INCLUDE_TESTS:BOOL=OFF would also likely reduce build times.

Copy link
Author

Choose a reason for hiding this comment

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

What's the option to enable shared build only? I've heard something about libllvm.so.
There may be other options that may reduce build time and image size, like selecting only the host machine's architecture (I think by default it's building a bunch of cross-compiling support as I can see it building stuff related to many archs in the log). Other options I don't know may limit which languages are supported.

Copy link
Owner

@mathstuf mathstuf Apr 8, 2019

Choose a reason for hiding this comment

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

-DBUILD_SHARED_LIBS:BOOL=ON

I think removing cross compilation makes sense. Removing TableGen compilation rules would certainly help compilation time. Likely link time/memory too.

Copy link
Author

Choose a reason for hiding this comment

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

OK. I got the install size of clang reduced by 10 times! (from 1.5GB down to 155MB). I played with the LVM_INCLUDE_TESTS and LVM_BUILD_TESTS options but it triggers a bug where a link commands wants the gtest lib, and it's not there. Anyway, the doc says LVM_BUILD_TESTS is OFF by default (the LVM_INCLUDE_TESTS seems to only control whether or not the generated ninja file includes rules for the test target or not).

Copy link
Owner

Choose a reason for hiding this comment

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

Sweet! I'm looking at reducing the GCC install size now. Probably don't need to compile non-C and C++ compilers…

@johan-boule johan-boule force-pushed the docker branch 7 times, most recently from ddea032 to 25ac3b2 Compare April 9, 2019 08:10
@mathstuf
Copy link
Owner

mathstuf commented Apr 9, 2019

I'm going to cherry-pick bits down into the branch I'm working on right now to respin. I have GCC down to < 250 MB :) .

@mathstuf mathstuf mentioned this pull request Apr 9, 2019
@mathstuf
Copy link
Owner

mathstuf commented Apr 9, 2019

Merged as part of #13. Thanks!

@mathstuf mathstuf closed this Apr 9, 2019
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.

None yet

2 participants