Allow specifying a custom generated header name by segiddins · Pull Request #350 · bazelbuild/rules_swift

segiddins · 2019-11-07T14:07:50Z

This is my first time contributing to this repo, and I couldn't find any guidelines on where or how to add tests.

This is an attempt to implement #346

googlebot · 2019-11-07T14:07:56Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

segiddins · 2019-11-07T17:29:41Z

~~(Working on getting added to our CLA google group)~~

CLA should be handled

segiddins · 2019-11-07T23:02:51Z

@googlebot I signed it!

googlebot · 2019-11-07T23:02:55Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

thii · 2019-11-08T01:46:06Z

swift/internal/api.bzl

+            "generated_header_name": attr.string(
+                doc = """\
+The name of the generated Objective-C interface header.
+If not provided, defaults to `${target_name}-Swift.h`.


The markdown documentation needs to be updated too. I think it's not auto-generated for now.

It's generated, just with a script that we run internally (because it's a bunch of hacks held together with chewing gum that we don't want to publish). Now that Stardoc has come farther along, it may be viable to switch over to.

tl;dr: don't worry about updating the docs in this PR, we'll regenerate them.

allevato · 2019-11-18T16:57:23Z

swift/internal/api.bzl

+            "generated_header_name": attr.string(
+                doc = """\
+The name of the generated Objective-C interface header.
+If not provided, defaults to `${target_name}-Swift.h`.


It's generated, just with a script that we run internally (because it's a bunch of hacks held together with chewing gum that we don't want to publish). Now that Stardoc has come farther along, it may be viable to switch over to.

tl;dr: don't worry about updating the docs in this PR, we'll regenerate them.

swift/internal/api.bzl

swift/internal/derived_files.bzl

swift/internal/api.bzl

swift/internal/swift_library.bzl

googlebot · 2019-12-03T18:24:52Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

segiddins · 2019-12-05T22:55:05Z

@allevato I think the CLA bot is complaining because of your commit via suggested changes

allevato · 2019-12-06T16:51:07Z

Yeah, you could work around that by squashing (I don't need to retain authorship of the suggestions) but since I have to pull the commit internally anyway to test it ultimately won't matter.

Sorry for the delay on this—I've been in an internal team event all week, so I'll likely take a look at this on Monday.

swift/internal/api.bzl

swift/internal/derived_files.bzl

Co-Authored-By: Tony Allevato <tony.allevato@gmail.com>

thii · 2020-01-08T10:52:57Z

Can we have this landed on the next release? :)

ob · 2020-01-10T23:51:40Z

Any updates on this?

thii

Found some errors when trying this patch (the validation always failed).

swift/internal/derived_files.bzl

-    return actions.declare_file("{}-Swift.h".format(target_name))
+    if header_name:
+        if not header_name.endswith(".h"):
+            fail(("The generated objc header name for {} must end in" +


swift/internal/derived_files.bzl

+            fail(("The generated objc header name for {} must end in" +
+                  " '.h', given '{}'") % (target_name, header_name))
+        if header_name.find("/"):
+            fail(("The generated objc header name for {} cannot contain a " +


Co-Authored-By: Thi <t@thi.im>

allevato · 2020-01-17T15:44:59Z

Sorry for the delay on this; in addition to being stretched a bit thin right now, I've been in the middle of refactoring these rules to support some future work that we need. I'll look at adding this change in as part of that so that you don't have to rebase it in on top of a completely changed implementation.

kastiglione · 2020-01-17T16:37:07Z

@allevato I was wondering with Keith what was driving the refactoring. Are you able to comment on the referenced "future work"?

allevato · 2020-01-17T18:48:22Z

@allevato I was wondering with Keith what was driving the refactoring. Are you able to comment on the referenced "future work"?

The old design was based around the assumption that the only action we cared about was SwiftCompile. When I prototyped the support to integrate PCMs into the dependency graph, it became clear that there were places where I collected info from providers where I'd need different subsets of flags/input files for the -emit-pcm action vs. the compile action, and trying to parameterize those functions to say "do these things but not these things" didn't scale at all.

By refactoring the actions to be more granular, adding PCM actions will be cleaner because all I'll have to do is add the PCM action name to the appropriate subset of action configurators, and then wire in the action itself.

It'll also pave the way for other improvements we've talked about but had to put off, like splitting out module compilation vs. object compilation.

kastiglione · 2020-01-18T01:41:21Z

thanks!

Co-authored-by: Samuel Giddins <segiddins@users.noreply.github.com> Closes #350. RELNOTES: None. PiperOrigin-RevId: 292413600

Co-authored-by: Samuel Giddins <segiddins@users.noreply.github.com> Closes bazelbuild#350. RELNOTES: None. PiperOrigin-RevId: 292541776

googlebot added the cla: no label Nov 7, 2019

googlebot added cla: yes and removed cla: no labels Nov 7, 2019

thii approved these changes Nov 8, 2019

View reviewed changes

allevato reviewed Nov 18, 2019

View reviewed changes

allevato reviewed Nov 19, 2019

View reviewed changes

swift/internal/swift_library.bzl Show resolved Hide resolved

segiddins force-pushed the segiddins/generated-header-name branch from 26230fa to 7469a85 Compare December 3, 2019 18:24

googlebot added cla: no and removed cla: yes labels Dec 3, 2019

allevato reviewed Dec 16, 2019

View reviewed changes

swift/internal/api.bzl Show resolved Hide resolved

swift/internal/derived_files.bzl Outdated Show resolved Hide resolved

segiddins and others added 5 commits December 18, 2019 17:06

Allow specifying a custom generated header name

68e05cf

Add missing arg doc

9a06196

Apply suggestions from code review

46893d2

Co-Authored-By: Tony Allevato <tony.allevato@gmail.com>

Only allow generated_header_name in swift_library attrs

46983e9

Validate given objc header name

3b51def

segiddins force-pushed the segiddins/generated-header-name branch from 7469a85 to 3b51def Compare December 19, 2019 01:08

thii reviewed Jan 13, 2020

View reviewed changes

ob mentioned this pull request Jan 13, 2020

Add example of mixed-source app and fix module name bug bazel-ios/rules_ios#10

Closed

Fix check for slash in header name

2189f3e

Co-Authored-By: Thi <t@thi.im>

swiple-rules-gardener pushed a commit that referenced this pull request Jan 30, 2020

Add a generated_header attribute to swift_library.

eee8d04

Co-authored-by: Samuel Giddins <segiddins@users.noreply.github.com> Closes #350. RELNOTES: None. PiperOrigin-RevId: 292413600

swiple-rules-gardener mentioned this pull request Jan 30, 2020

Add a generated_header attribute to swift_library. #385

Merged

swiple-rules-gardener pushed a commit that referenced this pull request Jan 31, 2020

Add a generated_header attribute to swift_library.

2f48b73

Co-authored-by: Samuel Giddins <segiddins@users.noreply.github.com> Closes #350. RELNOTES: None. PiperOrigin-RevId: 292413600

swiple-rules-gardener closed this in 4518085 Jan 31, 2020

swiple-rules-gardener closed this in #385 Jan 31, 2020

segiddins mentioned this pull request Aug 10, 2020

Allow generated headers with path separators #439

Merged

Conversation

segiddins commented Nov 7, 2019

Uh oh!

googlebot commented Nov 7, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

segiddins commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

segiddins commented Nov 7, 2019

Uh oh!

googlebot commented Nov 7, 2019

Uh oh!

thii Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

allevato Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

allevato Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

googlebot commented Dec 3, 2019

Uh oh!

segiddins commented Dec 5, 2019

Uh oh!

allevato commented Dec 6, 2019

Uh oh!

Uh oh!

Uh oh!

thii commented Jan 8, 2020

Uh oh!

ob commented Jan 10, 2020

Uh oh!

thii left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

allevato commented Jan 17, 2020

Uh oh!

kastiglione commented Jan 17, 2020

Uh oh!

allevato commented Jan 17, 2020

Uh oh!

kastiglione commented Jan 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

segiddins commented Nov 7, 2019 •

edited

Loading