Avoid umbrella header crash error in remote compilation when no default umbrella header generated

[gyfelton]

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 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.

[congt]

Nice findings!

[congt]

Could we always keep the else clause so that we always use FOUNDATION_EXPORT. It will be something like

if generate_default_umbrella_header:
    content += """\
#ifdef __OBJC__
#    import <Foundation/Foundation.h>
#    if __has_include(<UIKit/UIKit.h>)
#        import <UIKit/UIKit.h>
#    endif
#endif
"""

content += """\
#ifndef __OBJC__
#    ifndef FOUNDATION_EXPORT
#        if defined(__cplusplus)
#            define FOUNDATION_EXPORT extern "C"
#        else
#            define FOUNDATION_EXPORT extern
#        endif
#    endif
#endif
"""

Or something like

if generate_default_umbrella_header:
    content += """\
#ifdef __OBJC__
#    import <Foundation/Foundation.h>
#    if __has_include(<UIKit/UIKit.h>)
#        import <UIKit/UIKit.h>
#    endif
#else
#    ifndef FOUNDATION_EXPORT
#        if defined(__cplusplus)
#            define FOUNDATION_EXPORT extern "C"
#        else
#            define FOUNDATION_EXPORT extern
#        endif
#    endif
#endif
"""
else:
    content += """\
#ifndef __OBJC__
#    ifndef FOUNDATION_EXPORT
#        if defined(__cplusplus)
#            define FOUNDATION_EXPORT extern "C"
#        else
#            define FOUNDATION_EXPORT extern
#        endif
#    endif
#endif
"""

[gyfelton]

this will end having error: unknown type name 'FOUNDATION_EXPORT' error if we also replace extern with FOUNDATION_EXPORT.
this is because when generate_default_umbrella_header is off and the code has ifdef __OBJC__ is true, you won't get the FOUNDATION_EXPORT macro defined.

If we add just

#    ifndef FOUNDATION_EXPORT
#        if defined(__cplusplus)
#            define FOUNDATION_EXPORT extern "C"
#        else
#            define FOUNDATION_EXPORT extern
#        endif
#    endif

we end up having the warning: ambiguous expansion of macro 'FOUNDATION_EXPORT' error. which is sth i don't want

[congt]

That's weird. Could it be that we need to always define FOUNDATION_EXPORT extern when ifdef __OBJC__ is true (even if defined(__cplusplus) is true)? Could you try

if generate_default_umbrella_header:
    content += """\
#ifdef __OBJC__
#    import <Foundation/Foundation.h>
#    if __has_include(<UIKit/UIKit.h>)
#        import <UIKit/UIKit.h>
#    endif
#else
#    ifndef FOUNDATION_EXPORT
#        if defined(__cplusplus)
#            define FOUNDATION_EXPORT extern "C"
#        else
#            define FOUNDATION_EXPORT extern
#        endif
#    endif
#endif
"""
else:
    content += """\
#ifdef __OBJC__
#    define FOUNDATION_EXPORT extern
#else
#    ifndef FOUNDATION_EXPORT
#        if defined(__cplusplus)
#            define FOUNDATION_EXPORT extern "C"
#        else
#            define FOUNDATION_EXPORT extern
#        endif
#    endif
#endif
"""

That's essentially your current logic so not a big deal.

[gyfelton]

exactly, might as well be very explicit about the two code path this is going with