-
Notifications
You must be signed in to change notification settings - Fork 151
Explicitly pass .swiftmodule and .swiftinterface files as action inputs #1583
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
Open
adincebic
wants to merge
1
commit into
bazelbuild:master
Choose a base branch
from
adincebic:adin/fix-module-interface-dependency-compilation
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+151
−3
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
"""Rules for testing action inputs contain expected files.""" | ||
|
||
load("@bazel_skylib//lib:collections.bzl", "collections") | ||
load("@bazel_skylib//lib:unittest.bzl", "analysistest", "unittest") | ||
|
||
def _action_inputs_test_impl(ctx): | ||
env = analysistest.begin(ctx) | ||
target_under_test = analysistest.target_under_test(env) | ||
|
||
actions = analysistest.target_actions(env) | ||
mnemonic = ctx.attr.mnemonic | ||
matching_actions = [ | ||
action | ||
for action in actions | ||
if action.mnemonic == mnemonic | ||
] | ||
if not matching_actions: | ||
actual_mnemonics = collections.uniq( | ||
[action.mnemonic for action in actions], | ||
) | ||
unittest.fail( | ||
env, | ||
("Target '{}' registered no actions with the mnemonic '{}' " + | ||
"(it had {}).").format( | ||
str(target_under_test.label), | ||
mnemonic, | ||
actual_mnemonics, | ||
), | ||
) | ||
return analysistest.end(env) | ||
if len(matching_actions) != 1: | ||
unittest.fail( | ||
env, | ||
("Expected exactly one action with the mnemonic '{}', " + | ||
"but found {}.").format( | ||
mnemonic, | ||
len(matching_actions), | ||
), | ||
) | ||
return analysistest.end(env) | ||
|
||
action = matching_actions[0] | ||
message_prefix = "In {} action for target '{}', ".format( | ||
mnemonic, | ||
str(target_under_test.label), | ||
) | ||
|
||
input_paths = [input.short_path for input in action.inputs.to_list()] | ||
|
||
for expected_input in ctx.attr.expected_inputs: | ||
found = False | ||
for path in input_paths: | ||
if expected_input in path: | ||
found = True | ||
break | ||
if not found: | ||
unittest.fail( | ||
env, | ||
"{}expected inputs to contain file matching '{}', but it did not. Inputs: {}".format( | ||
message_prefix, | ||
expected_input, | ||
input_paths, | ||
), | ||
) | ||
|
||
for not_expected_input in ctx.attr.not_expected_inputs: | ||
found = False | ||
for path in input_paths: | ||
if not_expected_input in path: | ||
found = True | ||
break | ||
if found: | ||
unittest.fail( | ||
env, | ||
"{}expected inputs to not contain file matching '{}', but it did. Inputs: {}".format( | ||
message_prefix, | ||
not_expected_input, | ||
input_paths, | ||
), | ||
) | ||
|
||
return analysistest.end(env) | ||
|
||
def make_action_inputs_test_rule(config_settings = {}): | ||
"""A `action_inputs_test`-like rule with custom configs. | ||
|
||
Args: | ||
config_settings: A dictionary of configuration settings and their values | ||
that should be applied during tests. | ||
|
||
Returns: | ||
A rule returned by `analysistest.make` that has the `action_inputs_test` | ||
interface and the given config settings. | ||
""" | ||
return analysistest.make( | ||
_action_inputs_test_impl, | ||
attrs = { | ||
"mnemonic": attr.string( | ||
mandatory = True, | ||
doc = "The mnemonic of the action to test.", | ||
), | ||
"expected_inputs": attr.string_list( | ||
default = [], | ||
doc = "List of file patterns that should be present in action inputs.", | ||
), | ||
"not_expected_inputs": attr.string_list( | ||
default = [], | ||
doc = "List of file patterns that should not be present in action inputs.", | ||
), | ||
}, | ||
config_settings = config_settings, | ||
) | ||
|
||
# A default instantiation of the rule when no custom config settings are needed. | ||
action_inputs_test = make_action_inputs_test_rule() |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make it so
prerequisites.transitive_swiftmodules
has the right things (and probably rename it totransitive_included_modules
... though maybe we don't do that for now to keep cherry-picks easier). Also, we should only add one or the other. We should addswift_module.swiftmodule
if it exists, otherwiseswift_module.private_swiftinterface
, otherwiseswift_module.swiftinterface
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am running into issues if I pick only one of those files since
-swiftmodule
is getting generated even though it is not explicitly passed. I am assuming that.swiftmodule
is produced as a result of buildingswift_import
target with.swiftinterface
.Basically we need to keep logic as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we wouldn't compile
.swiftinterface
and just pass it along as an importable module. You should only need to compile a (private) interface into a module if you needed access to package/protected types, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really familiar with how
swift_import
rule is implemented. But essentially I am building https://github.com/wesprint-io/swift-syntax-prebuilt/releases and it only exposesswift_import
targets with no.swiftmodule
files yet they are produced at some point and if I pick either.swiftmodule
or.swiftinterface
the build fails because it won't pick up the.swiftinterface
since.swiftmodule
appears and it gets selected. Only way that I was able to make it work is by including both of them like in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brentleyjones given the following
swift_import
targetafter executing build
bazel build :SwiftBasicFormat
I see the following files producedI also added print staments to track this while build is in progress