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

Clang-cl support and various other improvements #520

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HC-pel
Copy link

@HC-pel HC-pel commented Mar 2, 2020

We are using Icecream extensively at HaCon. Over the years we developed various fixes and enhancements to better suit our needs. We recently rebased those changes on the latest release and want to share them now.

First of all our general setup and constraints, which may explain the need for some of the changes below:

  • All of our nodes are using Ubuntu 18.04
  • we are building with Clang 8 for both Linux and Windows
  • we provide our own builds for the compiler, which is not installed in a system directory
  • we build against libc++ for Linux and the Microsoft STL for Windows

The changes and additions are as follows:

  • Improved support for Clang
    • Better handling of custom-built binaries in non-system directories (e.g. handling relocating dependencies based on the RUNPATH)
    • The header files needed to be added to the Icecream environment, otherwise the remote build would fail due to undefined templates. This is despite the fact that the -fdirectives-only did properly concatenate all of the referenced headers
  • Support for building with clang-cl
    • This required changes to the preprocessor handling, as it outputs the include files through stdout instead of a dedicated .d file
    • It was primarily tested with our build configurations and may not support all parameters available in clang-cl
  • Stability improvements:
    • Fixed various crashes
    • handle abnormal compiler termination (this was not reported as an error to the client)
    • clean closing of sockets (there were broken pipe errors if one side clsoed the socket too early)
  • Misc:
    • New and extended messages to the monitor (currently only processed by our own monitor tool, but available for use in Icemon as well)
    • No file locks for remote preprocessor. This was the cause of a lot of "broken pipe" errors, as local builds may block the lock long enough for the remote to close the connection
    • set startup time for ICECC_FAKE_STARTTIME to January 1 2000 instead of a certain number of seconds in the past
    • fix daemon log file getting overwritten by daemon crash dump
    • Schedulers only broadcast if they are accessible from the outside, fixes weird behavior if a firewall blocks access

The pull request currently contains all the changes squashed together. We are aware this is not an ideal way to review the changes, it is the result of a difficult rebase to the latest version. We want to use this pull request to discuss which changes make sense for the upstream version of Icecream. The attached commit can be seen as a reference/preview of what exactly the changes are. Once we know which bits are relevant we can attempt to create cleaner commits separated into the individual features, changes and fixes - and probably also handled in smaller pull requests.

To note is also that we defined our own Icecream protocol versions starting at 100. The related checks will be updated to the next-lowest available official protocol version as well.

We are looking forward to your feedback!

@llunak
Copy link
Contributor

llunak commented Mar 8, 2020

Yes, we generally are interested in any changes that are generally usable, and indeed it should not be done as one big patch.

Comments on the parts I see in the commit:

  • Adding internal Clang headers to the tarball in client/icecc-create-env.in used to be done, but it was removed by do not package Clang internal headers in icecc-create-env #473 , and it doesn't seem to be actually needed. Why is it necessary for you?
  • ICECC_CLANG_REMOTE_CPP
  • ICECC_TEST_REMOTEBUILD - if you want to force a remote compile, ICECC_PREFERRED_HOST= should do the job, so this one shouldn't be necessary.
  • clang-cl support - How can you use clang-cl with Icecream? Do you cross-compile on Linux to Windows? The changes do not look that big, so if it's of general use it can be included, but that's what I'm not sure about, it may be a rather niche use. Which, among other things, means that it could get broken regularly but everybody else not using clang-cl, unless the patches would include tests making sure it keeps working.

But there are more changes in the diff and it's difficult to review this way without spending a lot of time on it, I'm not even sure which changes belong together. I suggest you start with small things that should be simple, such as the various fixes, and then continue with the more complicated ones. This way it'll be simpler for both sides to start doing something with it, and as it'll progress it'll be easier to see what belongs upstream and what is probably not worth it.

@HC-pel
Copy link
Author

HC-pel commented Mar 19, 2020

Sorry for the long delay. We split up the changes into smaller commits now: https://github.com/HC-pel/icecream/commits/individual
We'll create separate pull requests for the various changes.

About clang-cl: We are using it for cross compiling, yes. It is a somewhat niche usage. However, with clang-cl cross compiling Visual Studio compatible binaries has become much simpler, so for projects targetting both platforms it could be very useful. I agree that there should be tests though, we'll look into that.

I do distinctly remember having to add header files into the tarball to work around compilation errors. I'm not sure if it was fixed by this specific change, however. It's possible that it was accidently reintrooduced in the rebase. I'll analyze what exactly is necessary.

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.

2 participants