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

fix: Provide labels that are compartible with rules_python #89

Conversation

ewianda
Copy link
Contributor

@ewianda ewianda commented Apr 16, 2024

Provide labels that are compatible with rules-python and python gazelle

@@ -201,8 +206,9 @@ def _package_repo_impl(rctx):
_generate_lock_bzl(rctx, lock_json_path, lock_bzl_path)

for pin, pin_target in lock["pins"].items():
pin = pin.replace("-", "_")
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 is a breaking change, not sure how this could be handled. Just wanted to demonstrate that it is possible.

@jvolkman
Copy link
Owner

Thanks. I actually gave the current layout a good amount of thought. Some of those thoughts are documented in comments in package_repo.bzl.

The //foo/... namespace is reserved for things related to package foo that aren't the library itself. Currently the only items are wheel and sdist, but in the future my plan was to add scripts and other things. This change would generate a conflicting target for the wheel package, since //wheel:wheel already exists and means "the .whl file for the 'wheel'".

And for the dash vs. underscore situation: I decided to stick with python's own package normalization (described here), which is fully compatible with Bazel target names.

So I won't accept this as is. Any change would have to be some alternative, configurable layout type, or some additional repo that can be added on top. Ideally we could just tell Gazelle how to find packages somehow, rather than it assuming a particular naming scheme.

@ewianda
Copy link
Contributor Author

ewianda commented Apr 16, 2024

Telling Gazelle how to handle labels is a fair point.

What are your thoughts on handling py_console_script_binary

@jvolkman
Copy link
Owner

What are your thoughts on handling py_console_script_binary

My plan is to put package binaries under //foo/bin:some_tool.

But unlike rules_python where wheel contents can be analyzed in repository rules (and thus targets for entrypoint scripts can be created), pycross can't scan wheels and create targets in build rules. So any binaries/scripts that will be available under bin will need to be called out by the user ahead of time.

@ewianda
Copy link
Contributor Author

ewianda commented Apr 18, 2024

For how I am using this workaround to get the py_console_script going

load("@rules_python//python/entry_points:py_console_script_binary.bzl", _py_console_script_binary = "py_console_script_binary")

def py_console_script_binary(name, **kwargs):
    """
    This is a workaround for the fact that rules_pycross doesn't provide dist-info targets.
    name: Name of the binary target.
    **kwargs: Arguments to pass to py_console_script_binary.
    """
    genrule_name = "generate_{}_entrypoint_points_txt".format(name)

    native.genrule(
        name = genrule_name,
        srcs = [kwargs["pkg"]],
        outs = ["{}.dist-info/entry_points.txt".format(name)],
        cmd = """
        cat $$(find $(SRCS) -name entry_points.txt) > $(@D)/entry_points.txt
        """,
    )

    _py_console_script_binary(
        name = name,
        entry_points_txt = genrule_name,
        **kwargs
    )

@ewianda ewianda closed this Apr 18, 2024
@wingsofovnia
Copy link
Contributor

wingsofovnia commented May 27, 2024

@ewianda have you figured out how to make rules_python's Gazelle plugin compatible with rules_pycross targets? Perhaps there is a related issue in rules_python repo?
upd: created an issue #100 for discussing further steps

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.

3 participants