Skip to content

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Mar 1, 2018

No description provided.

@pitrou
Copy link
Member Author

pitrou commented Mar 1, 2018

Example AppVeyor build at https://ci.appveyor.com/project/pitrou/arrow/build/1.0.155

@pitrou
Copy link
Member Author

pitrou commented Mar 1, 2018

The failure at https://ci.appveyor.com/project/pitrou/arrow/build/1.0.155/job/q31movster4v84d9 shows this can lead to inconsistencies or errors: cmake first tries to detect the compller from user-supplied information (generator, environment variables), then the clcache setting overrides that detection.

Either we add logic to try and avoid such errors, or we simply let people override CC/CXX if they want to use clcache (statu quo).

@pitrou
Copy link
Member Author

pitrou commented Mar 1, 2018

Also I'm not sure whether we have a Windows developer on board; I'm merely launching a VM from time to time but otherwise work on Ubuntu :-)

@wesm
Copy link
Member

wesm commented Mar 1, 2018

@Maxris can take a look. How does the error you linked to arise?

@pitrou
Copy link
Member Author

pitrou commented Mar 1, 2018

I think that's because CMAKE_CXX_COMPILER forcefully overrides the compiler command. When using the Visual Studio generators, you traditionally don't need to run vcvarsall.bat (presumably because cmake would hardcode the full compiler path), but then clcache fails finding the compiler.

So it's possible that calling vcvarsall.bat is all that's needed here. But that would also change the workflow people may be accustomed to.

@maxhora
Copy link
Contributor

maxhora commented Mar 1, 2018

@pitrou I will check, thanks

@maxhora
Copy link
Contributor

maxhora commented Mar 1, 2018

@pitrou it seems that we already try to use ccache there if it's presented. I'm wondering if it will make more sense to refactor referenced lines and optionally use clcache for MSVC?
Also, probably, usage of RULE_LAUNCH_COMPILE and RULE_LAUNCH_LINK should solve issue with selected compiler overwrite.
And it seems that starting from Cmake 3.4.0 CXX_COMPILER_LAUNCHER variable is available, but we stick to CMake of min ver 3.2

@pitrou
Copy link
Member Author

pitrou commented Mar 1, 2018

Also, probably, usage of RULE_LAUNCH_COMPILE and RULE_LAUNCH_LINK should solve issue with selected compiler overwrite.

Last I tried it seemed it didn't work. I might give it a try again...

@maxhora
Copy link
Contributor

maxhora commented Mar 1, 2018

I will try on my end as well

@maxhora
Copy link
Contributor

maxhora commented Mar 1, 2018

@pitrou, do you have an idea how to verify that clcache.exe was really used during compilation? I've tried with it and without, but I can't find any difference in output/produced results.

@pitrou
Copy link
Member Author

pitrou commented Mar 2, 2018

@Maxris running clcache -s gives you aggregate statistics for the cache, so you can see (by the number of hits and misses) if clcache was used at all.

@maxhora
Copy link
Contributor

maxhora commented Mar 2, 2018

@pitrou thanks!

@maxhora
Copy link
Contributor

maxhora commented Mar 4, 2018

update: it seems that current solution set(CMAKE_CXX_COMPILER ${CLCACHE_FOUND}) works only with NMake Makefiles generator, but clcache doesn't get called if Visual Studio 14 2015 Win64 or similar is used.

@pitrou
Copy link
Member Author

pitrou commented Mar 4, 2018

clcache works best with Ninja or NMake (*). My suggestion here would be to recommend Ninja + clcache for best build performance. The other concern, though, is to avoid breaking existing builds for those who prefer other generators (e.g. Visual Studio).

(*) See the following links:

@maxhora
Copy link
Contributor

maxhora commented Mar 4, 2018

@pitrou it should be fine to set set(CMAKE_CXX_COMPILER ${CLCACHE_FOUND}) only if Generator defined as Ninja or NMake Makefiles. This possibly also will resolve current Appveyor failure. What do you think?

@pitrou
Copy link
Member Author

pitrou commented Mar 4, 2018

@Maxris that sounds ok to me.

@maxhora
Copy link
Contributor

maxhora commented Mar 4, 2018

@pitrou I will check if that resolves Appveyor build failure and let you know, thanks

@maxhora
Copy link
Contributor

maxhora commented Mar 5, 2018

@pitrou here it's changes to solve Jenkins failure as discussed
Passed Appveyor build

P.S. I've tried to create PR into your pitrou:ARROW-2238-cmake-clcache remote branch, but for some reasons it's not listed in targets during PR creation.

@pitrou
Copy link
Member Author

pitrou commented Mar 6, 2018

@Maxris thanks! I'll integrate it in my PR.

@pitrou pitrou force-pushed the ARROW-2238-cmake-clcache branch from 673a82e to 8539a0e Compare March 6, 2018 12:26
@pitrou
Copy link
Member Author

pitrou commented Mar 6, 2018

Actually, it doesn't look right. Here you see that the cache wasn't looked up at all:
https://ci.appveyor.com/project/MaxRisuhin/arrow/build/1.0.253/job/puv48c73yknmel34#L3175

@maxhora
Copy link
Contributor

maxhora commented Mar 6, 2018

@pitrou sorry, haven't checked in such manner. I have checked only that clcache was applied as compiler replacement with additional output https://ci.appveyor.com/project/MaxRisuhin/arrow/build/1.0.253/job/e8v49o78ku8cltur#L418 ... and also have seen that with NMake Makefiles generator clcache is used in local builds. Will try to figure out why it doesn't work in Appveyor env.

@maxhora
Copy link
Contributor

maxhora commented Mar 6, 2018

Hey @pitrou , could you please check logs of last passed build ( https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/build/1.0.5601 ). For some reasons clcache statistics shows that it was used, by my opinion, fine. Could you please verify? Thanks
P.S. It also should be normal that JOB=Cmake_Script_Tests doesn't generate cache entries.

@pitrou
Copy link
Member Author

pitrou commented Mar 6, 2018

@Maxris, yes, it looks fine... And the build times are ok too (without cache, the total duration would probably be much more than 45 minutes).

@maxhora
Copy link
Contributor

maxhora commented Mar 6, 2018

Great! Should be fine to go with these changes, thanks!

@xhochy
Copy link
Member

xhochy commented Mar 7, 2018

@Maxris @pitrou This also looks fine from my side but as you both are more qualified on that matter: Is this ready to be merged?

@pitrou
Copy link
Member Author

pitrou commented Mar 7, 2018

@xhochy, yes, it's ready for merging.

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1

@wesm wesm closed this in fb2316c Mar 7, 2018
@pitrou pitrou deleted the ARROW-2238-cmake-clcache branch March 7, 2018 15:50
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.

4 participants