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

Fixes for the macOS build (iPhone with Xcode) #555

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cinakyn
Copy link
Contributor

@cinakyn cinakyn commented Jul 28, 2020

Hi.
I have used dozen of days for building iPhone application using icecc.
During works, There was some needs to modify little codes for handling clang and iPhone target.
This PR is the result of that modifications.
I hope these are helpful for other icecc user but it could be not.

My Environment:

  • Three macOS machines.
    • 1 macbook/ 2 macmini
    • all machines are using 10.15.5 catalina
  • Xcode Version 12.0 beta 2 (12A6163b)
    • I had to use the clang which contains this commit. Cause the iPhoneSDK is using __has_include to decide wether some frameworks are included or not.
    • Xcode 12 is the only version contains apple-llvm containing that commit

Commits

8e551a6

Make some flags to be local because they are only used by preprocessor or would be OK without them.

  • serialize-diagnostics: Might be used to save diagnostic result into serialized format.
  • others: Might be used to mange the clang module build status. But I couldn't fully understand sorry.
27f52dc

clang compilation with "-fembed-bitcode" tries to access /var/tmp directory. As icecc environment maker doesn't handle it, iPhone build always fail with message clang: unable to make temporary file: No such file or directory.. This commit make dummy files for keeping that directory.

325aee7

This commit removed handling pch for clang. Same logic exist on call_cpp is making some troubles. So I removed it from code.

Thanks.

Copy link
Contributor

@llunak llunak left a comment

Choose a reason for hiding this comment

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

I don't understand the reasoning for the removal of the PCH part, I've added inline comments.

tests/test.sh Outdated
@@ -2206,7 +2206,7 @@ else
fi

run_ice "$testdir/includes.h.gch" "local" 0 "keepoutput" $TESTCXX -x c++-header -Wall -Werror -c includes.h -o "$testdir"/includes.h.gch
run_ice "$testdir/includes.o" "remote" 0 $TESTCXX -Wall -Werror -c includes.cpp -include "$testdir"/includes.h -Winvalid-pch -o "$testdir"/includes.o
run_ice "$testdir/includes.o" "remote" 0 $TESTCXX -Wall -Werror -c includes.cpp -include includes.h -Winvalid-pch -o "$testdir"/includes.o
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need changing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original:
-include and "$testdir"/includes.h is actually ignored by codes I removed in the 325aee7. So test pass successfully.

Modified:
325aee7 makes "$testdir"/includes.h starts actully to work. But there's no file with that name. Only includes.h or `"$testdir"/includes.h.gch' are exist.
So I fixed it working as I have thought it is came from mistake.

client/cpp.cpp Outdated
o = it++;
flags.erase(o);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What trouble exactly is this part supposed to cause? The flags are removed for a reason, PCH files are not sent to the remote node, so if the flags are kept, the remote compilation may fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my environment, that code made building fail.
I'm using Prefix Header option on Xcode.
This option help for not including common headers which is always included.
For example, As I set prefixHeader.pch as value of Prefix Header option, I don't need to include stdarg or vector or any other in my sourcecode.

// contents of prefixHeader.pch
#include <stdarg.h>
#include <stdlib.h>
#include <string.h>
#include <limits.h>
#include <float.h>
#include <math.h>
#include <stdio.h>
#include <time.h>
#include <stdint.h>
#include <assert.h>
#include <sys/time.h>
#include <sys/select.h>
#include <OpenGLES/ES2/gl.h>
#include <OpenGLES/ES2/glext.h>

#ifdef __OBJC__
    #import <Foundation/Foundation.h>
    #import <UIKit/UIKit.h>
#endif

#ifdef __cplusplus
    #include <vector>
    #include <string>
    #include <map>
    #include <list>
    #include <memory>
    #include <functional>
    #include <thread>
    #include <mutex>
    #include <unordered_map>
    #include <list>
    #include <algorithm>
    #include <cstdint>
    #include <stack>
#endif

But code that I removed make "-include prefixHeader.pch" skipped during local compilation, So build is failed because that source has not included stdio or vector or etc...

After some investigation, I found you removed the smilar code(#475 ).
I have thought the comment in the #475 can be also applied to the code I removed.
If it must not be removed, I have to find other way to support Prefix Header option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bleows are documentations in the Xcode.

Precompile Prefix Header (GCC_PRECOMPILE_PREFIX_HEADER)
Generates a precompiled header for the prefix header, which should reduce overall build times.

Precompiling the prefix header will be most effective if the contents of the prefix header or any file it includes change rarely. If the contents of the prefix header or any file it includes change frequently, there may be a negative impact to overall build time.

Prefix Header (GCC_PREFIX_HEADER)
Implicitly include the named header. The path given should either be a project relative path or an absolute path.

Preprocessor Macros (GCC_PREPROCESSOR_DEFINITIONS)
Space-separated list of preprocessor macros of the form foo or foo=bar.

@cinakyn
Copy link
Contributor Author

cinakyn commented Jul 30, 2020

@llunak
Thank you for commenting.
I just have repiled to your comment.
I regret not to have good english skills.

++ I have found the commit hash in the description is wrong. I have fixed it.
++ I have added another small commit for option appeared when build using xcode gui

@cinakyn
Copy link
Contributor Author

cinakyn commented Aug 11, 2020

@llunak
Hi, I have reverted the commit 325aee7.
You're right. it was my mistake.
My problem have been occurred when...

  1. On the Xcode,
  2. Use ios/Prefix.pch as Prefix Header. As my source code might not include the ios/Prefix.pch, it never be compiled without -include ios/Prefix.pch
  3. Turn Precompile Prefix Header On. It would make /path/to/intermediate/Prefix.pch during build process. And change the -include ios/Prefix.pch into -incldue /path/to/intermediate/Prefix.pch
  4. local daemon has chance to not have /path/to/intermediate/Prefix.pch as it could be built from remote machine.
  5. In that case, (access(p.c_str(), R_OK) < 0 && access((p + ".gch").c_str(), R_OK) == 0) will be true.
  6. So my local daemon removes -incldue /path/to/intermediate/Prefix.pch from my command making my source code never be compiled as "step 2." saids.

Conclusion:
I would have to Turn Precompile Prefix Header Off not commiting 325aee7
Thanks.

@deriamis
Copy link

deriamis commented Jun 12, 2022

@cinakyn I'm going to put this behind #548 because it looks like at one point you're trying to do the same sort of thing - create a "fake" path and include it in the compiler tarball. I would like to see #548 become a general mechanism for doing that. I'll take a further look at this PR over the coming weeks to see if it's good to go after we get the other one in.

@deriamis deriamis self-assigned this Jun 12, 2022
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