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

Make rules_swift work with virtual import directories. #298

Merged
merged 6 commits into from
Aug 30, 2019

Conversation

lberki
Copy link
Contributor

@lberki lberki commented Aug 29, 2019

This is necessary for the rule set to work with
--incompatible_generated_protos_in_virtual_imports (and also with
proto_library rules with strip_import_prefix / import_prefix)

This is necessary for the rule set to work with
`--incompatible_generated_protos_in_virtual_imports` (and also with
`proto_library` rules with `strip_import_prefix` / `import_prefix`)
@thomasvl
Copy link
Member

From the bazel issue, I thought the right way of doing this was going to be using the root from the provider and not hardcoding things?

@lberki
Copy link
Contributor Author

lberki commented Aug 29, 2019

Do I understand correctly that I meticulously told you how to do things the right way then I did them the wrong way anyway? I'm talented like that...

Fixed, PTAL.

Copy link
Member

@thomasvl thomasvl left a comment

Choose a reason for hiding this comment

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

@lberki
Copy link
Contributor Author

lberki commented Aug 29, 2019

I'd be happy to, but how do I know if I succeeded? The tests pass with this change, too.

@thomasvl
Copy link
Member

I'd be happy to, but how do I know if I succeeded? The tests pass with this change, too.

This is one of the problems trying to write tests for rules/bazel. You need a custom workspace with another repo reference that uses the rules. They are pretty hard to set up, so most rules sets don't seem to have them.

@lberki
Copy link
Contributor Author

lberki commented Aug 29, 2019

So what do I do? Is there any prior art as to what needs to work? (I thought //examples/... would be enough, which is what Bazel's BuildKite instance does)

@lberki
Copy link
Contributor Author

lberki commented Aug 30, 2019

Updated the above places and checked whether rules work when imported from a remote repository and with generated .proto files. Please take another look!

@@ -88,7 +91,7 @@ def register_module_mapping_write_action(name, actions, module_mappings):

return mapping_file

def _generated_file_path(name, extension_fragment, proto_file = None):
def _generated_file_path(name, extension_fragment, proto_source_root, proto_file = None):
"""Returns the short path of a generated `.swift` file corresponding to a `.proto` file.

The returned workspace-relative path should be used to declare output files so that they are
Copy link
Member

Choose a reason for hiding this comment

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

Is this part still true, or is the output path now slightly different since it might not have some the the original workspace relative info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that this is still true -- the format is PACKAGE/NAME.protoc_gen_EXTENSION_swift/IMPORT_PATH.<something>, to which all of this is true. Except that the path now contains the import path and not the workspace-relative path of the source file.

@@ -251,3 +251,20 @@ def workspace_relative_path(file):
"""
Copy link
Member

Choose a reason for hiding this comment

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

Is anything left using workspace_relative_path or should it be made private to this file?

Copy link
Contributor Author

@lberki lberki Aug 30, 2019

Choose a reason for hiding this comment

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

You cannot know that -- anyone can load() this file and use this function from it. If you wish, I can remove it, but be aware that it's a potential incompatible change.

Copy link
Member

Choose a reason for hiding this comment

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

You cannot know that -- anyone can load()s this file and use this function from it. If you wish, I can remove it, but be aware that it's a potential incompatible change.

Actually, it is in an internal directory, so we're fine breaking folks that directly tried to use it. It just needs to be not used by anything else within rules_swift. The directory naming is all we can do until bazel has visibility for bzl files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So be it then!

swiple-rules-gardener added a commit that referenced this pull request Aug 30, 2019
PiperOrigin-RevId: 266443442
@swiple-rules-gardener swiple-rules-gardener merged commit 6d5a84d into bazelbuild:master Aug 30, 2019
tymurmustafaiev pushed a commit to tymurmustafaiev/rules_swift that referenced this pull request Jul 19, 2023
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