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

Refer to actual generated swift header when making swift hmap #48

Merged
merged 2 commits into from
Apr 21, 2020

Conversation

segiddins
Copy link
Member

Before, the file in hdrs referred to a non-existant source file

We can see the fix in effect by the xcodeproj build not failing, since the -Swift.h header is no longer getting added to the xcode project !

cc @wileykestner

Before, the file in hdrs referred to a non-existant source file
@segiddins segiddins requested a review from amberdixon April 21, 2020 01:42
rules/hmap.bzl Show resolved Hide resolved
"-fmodules",
"-fmodule-name=%s" % module_name,
"-gmodules",
]

swift_copts += [j for i in ([["-Xcc", copt] for copt in headermap_copts]) for j in i] + [
swift_copts += [
"-Xcc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems like you already added -Xcc on line 448.

Copy link
Member Author

Choose a reason for hiding this comment

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

You need an -Xcc for every argument that you want passed to clang

@@ -1,3 +1,3 @@
# We can't create a bzl_library for rules-swift because of its visibility,
# so circumvent by not using the sandbox
common --strategy=Stardoc=standalone
build --strategy=Stardoc=standalone
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this setting need to change?

Copy link
Member Author

Choose a reason for hiding this comment

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

--strategy is apparently not valid for query

Copy link
Contributor

Choose a reason for hiding this comment

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

thx for fixing this! I just ran into it

Copy link
Contributor

@gyfelton gyfelton left a comment

Choose a reason for hiding this comment

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

agree with comments Amber puts

Copy link
Member

@ob ob left a comment

Choose a reason for hiding this comment

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

Good cleanup! Thanks!

all_hdrs += provider.files.to_list()
all_hdrs = list(ctx.files.hdrs)
for provider in ctx.attr.direct_hdr_providers:
if apple_common.Objc in provider:
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I assume swift rules provide the -Swift.h header in an objc provider now no?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, they're in direct_headers

Copy link
Contributor

@wileykestner wileykestner 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 think I am qualified at this point to approve this PR or not, but I did leave a question to see if I'm generally following along…

The only other question I have is:

Was the problem we were trying to solve here that we had an extra -Swift.h file? If so, is it possible to write a test to show that we no longer generate an extra -Swift.h file?

name = swift_doublequote_hmap_name,
namespace = namespace,
hdrs = [],
direct_hdr_providers = [swift_libname],
Copy link
Contributor

@wileykestner wileykestner Apr 21, 2020

Choose a reason for hiding this comment

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

Echoing @ob's question from above, I assume what we're doing here is passing an instantiated swift_library rule into the headermap rule. In my beginner Bazel adopters mind, what's happening here isn't that we are passing some artifact, we are passing the information that the instantiated swift_library rules wants its consumers it to have: in this case, swift_library generates a -Swift.h header. In Bazel terms, the information an instantiated rule wants its consumers to be able to access is called a Provider, and Providers can be thought of as the "return value" of the instantiating a rule.

In this case, this headermap rule would like to include information about the instantiated swift_librarys -Swift.hheader into a header map, so that ObjC classes in targets that consume this apple_library can call methods on the Swift classes that the swift_library exposes in its header.

Is that more or less what's happening here?

Copy link
Member

Choose a reason for hiding this comment

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

That is exactly what’s going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome. Thanks for confirming @ob!

@segiddins
Copy link
Member Author

Was the problem we were trying to solve here that we had an extra -Swift.h file? If so, is it possible to write a test to show that we no longer generate an extra -Swift.h file?

It wasn't actually generated, but the hmap had a reference to a source file with the name ModuleName-Swift.h. That file didn't exist, but we didn't catch it because nothing in the hmap rule actually uses the headers as inputs to any actions, so bazel was happy to continue passing it along, even though it didn't exist.

This showed up in the Xcodeproj rule, since that rule gathers all transitive source (e.g. not generated) files, and the -Swift.h file was showing up there, even though it (should have been a) generated file.

@segiddins segiddins merged commit f8823a6 into master Apr 21, 2020
@segiddins segiddins deleted the segiddins/refer-to-actual-generated-swift-header branch April 21, 2020 21:45
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.

5 participants