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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Aug 21, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions rules/library.bzl
View file
Open in desktop
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__
Copy link
Contributor

@congt congt Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@congt congt Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

# 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice findings!

{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