Skip to content

Commit

Permalink
Avoid umbrella header crash error in remote compilation when no defau…
Browse files Browse the repository at this point in the history
…lt umbrella header generated (#897)

### What changed:
1. When generate_default_umbrella_header is set to False, still add
statements around `extern double {module_name}VersionNumber;` to
umbrella header so that clang can resolve the situation when module_name
is the same for two different modules (where their path is diff).
2. When generate_default_umbrella_header is set to False, use `extern`
instead of `FOUNDATION_EXPORT` since `FOUNDATION_EXPORT` is `extern` and
trying to define it in `else` clause will cause `warning: ambiguous
expansion of macro 'FOUNDATION_EXPORT'` error.

### Why this change:
[The original PR](#348) that
introduced the flag `generate_default_umbrella_header` essentially wants
to not import UIKit/Foundation to restrict on what to import by default,
which is a good thing. However, it kinda overkill by also not including
the statements around version number and version string. Which
apparently affects how clang resolves module mapping (from a module name
to a path) and in our set up, leading to error similar to:
```
path/to/alice.modulemap:2:21: error: umbrella for module 'bob' already covers this directory

   umbrella header "foo-umbrella.h"
                    ^
```
For reference, this is originated from `diag::err_mmap_umbrella_clash`
error in https://github.com/llvm/llvm-project

For our case, the umbrella header name for both alice and bob modules
are `foo`, which is totally fine since their path is different. But for
some reason when trying to compile the code in a remote execution
service such as buildfarm, above error shows up. I cannot reproduce it
with `local` or `sandboxed` mode.

Now looking back, the theory is that when there is clash on umbrealla
header name, it will use the module's version string/number to figure
out which is which? I don't really have any other theory for now. I
cannot find such reference to `VersionNumber`/`VersionString` that is
meaningful. Nor can I setup a test case here since it only break remote
execution.

This does not revert the original PR in anyway, I still get to
implicitly import Foundation/UIKit with this change.

### Tests
No change on testing since this flag was not tested anywhere for now. I
can add this flag to some random testing targets in this PR though but
tests so far are still all passing with this turned off.
  • Loading branch information
gyfelton authored Aug 21, 2024
1 parent 98d49d5 commit a15336e
Showing 1 changed file with 8 additions and 7 deletions.
15 changes: 8 additions & 7 deletions rules/library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,9 @@ def _write_umbrella_header(
module_name = name

content = ""

extern_keyword = "extern"
if generate_default_umbrella_header:
extern_keyword = "FOUNDATION_EXPORT"
content += """\
#ifdef __OBJC__
# import <Foundation/Foundation.h>
Expand All @@ -165,13 +166,13 @@ def _write_umbrella_header(
for header in public_headers:
content += "#import \"{header}\"\n".format(header = paths.basename(header))

if generate_default_umbrella_header:
content += """
FOUNDATION_EXPORT double {module_name}VersionNumber;
FOUNDATION_EXPORT const unsigned char {module_name}VersionString[];
content += """
{extern_keyword} double {module_name}VersionNumber;
{extern_keyword} const unsigned char {module_name}VersionString[];
""".format(
module_name = module_name,
)
extern_keyword = extern_keyword,
module_name = module_name,
)

write_file(
name = basename + "~",
Expand Down

0 comments on commit a15336e

Please sign in to comment.