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

Fix building with USECCACHE=1. #8906

Merged
merged 1 commit into from
Nov 6, 2014
Merged

Fix building with USECCACHE=1. #8906

merged 1 commit into from
Nov 6, 2014

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Nov 5, 2014

After 6300cbe, building with USECCACHE=1 was broken due to the alterations to CC and CXX, causing $(BINARY) to expand badly and ending up with -m rather than -m64 in the compilation command.

This fixes it, although to be honest I'm not completely sure about this. Maybe some Make guru can comment on whether this is the proper way to prepend ccache to the recursively expanding CC and CXX variables.

@tkelman
Copy link
Contributor

tkelman commented Nov 5, 2014

Have you tried a build from scratch with these set? I'm almost positive it'll fail at building libgit, since cmake is picky about not having flags in the compiler names. According to http://stackoverflow.com/questions/1815688/how-to-use-ccache-with-cmake we probably want to use one of the ccache symlinks in /usr/lib/ccache.

@maleadt
Copy link
Member Author

maleadt commented Nov 5, 2014

You're right, I only tested a make after cleanall.

The problem with the ccache symlinks is that you can't rely on them existing, especially in case of clang. And creating an in-tree symlink clang->ccache seems a bit of a hack.

Maybe checking if CC/CXX is multiword, and if so, and splitting into CMAKE_CXX_COMPILER and CMAKE_CXX_COMPILER_ARG1? I'll have a try.

@maleadt
Copy link
Member Author

maleadt commented Nov 5, 2014

Would you prefer something like:

 ifeq ($(USECCACHE), 1)
-CC := ccache $(CC)
-CXX := ccache $(CXX)
+CC_BASE  := $(JULIAHOME)/ccache/$(CC)
+CXX_BASE := $(JULIAHOME)/ccache/$(CXX)
+$(shell mkdir -p $(JULIAHOME)/ccache)
+$(shell ln -sf `which ccache` $(CC_BASE))
+$(shell ln -sf `which ccache` $(CXX_BASE))
+CC = $(CC_BASE)
+CXX = $(CXX_BASE)

Seems terribly fragile though, for example when using a non-trivial CC (e.g. CC=/usr/bin/local/gcc-4.2)

@tkelman
Copy link
Contributor

tkelman commented Nov 5, 2014

Did CMAKE_CXX_COMPILER_ARG1 stuff not work? I hadn't heard of that cmake flag before, but that stack overflow post didn't look too encouraging.

For making our own symlinks, anything you can get to work after a make -C deps distclean-libgit2 && make cleanall would probably be fine by me. I've never used ccache and you're the first person to complain that I broke it, so I think you can be the authority on it now. :)

@maleadt maleadt changed the title Fix variable expansion in case of USECCACHE=1. Fix building with USECCACHE=1. Nov 6, 2014
@maleadt
Copy link
Member Author

maleadt commented Nov 6, 2014

Okay, I pushed my second take on this, this time using CMAKE_CXX_COMPILER_ARG1. Even though the SO post wasn't too positive about that option, I have been using it for quite a while without any issue, and it seems to work well for libgit2 too. And it's quite a bit cleaner than the symlink solution :)

I haven't tested the libcxx building, but it follows the same principle so should work fine.

@maleadt
Copy link
Member Author

maleadt commented Nov 6, 2014

Why doesn't this build? The Linux bot doesn't seem to list an error, and the Mac error doesn't really ring a bell...

@jiahao
Copy link
Member

jiahao commented Nov 6, 2014

#8914

This fixes the variable expansion and building with CMake.
@maleadt
Copy link
Member Author

maleadt commented Nov 6, 2014

Rebased on top of master, and Travis seems all-right now. Except for the Mac bot, but that seems like a failing brew command.

@tkelman
Copy link
Contributor

tkelman commented Nov 6, 2014

This looks good enough to me. I even installed ccache and it seems to have worked fine. Thanks for fixing it.

tkelman added a commit that referenced this pull request Nov 6, 2014
Fix building with USECCACHE=1.
@tkelman tkelman merged commit 4119816 into JuliaLang:master Nov 6, 2014
@maleadt
Copy link
Member Author

maleadt commented Nov 6, 2014

Mostly scratching my own itches :) Thanks for merging.

@maleadt maleadt deleted the pr_ccache branch November 6, 2014 21:32
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