Skip to content

Conversation

@aignas
Copy link
Collaborator

@aignas aignas commented Aug 22, 2024

WIP - this is to discuss an interface for allowing users to better customize
the build process. Once this is defined, we can then implement something for
allowing users to build from source in the build context. That would require
the following missing pieces that should be implemented in other PRs:

  • Using http_archive or implement a new sdist_library repository rule
    for handling sdist.
  • A rudimentary py_sdist_library that allows invoking a PEP518 build
    backend in the build phase.

Related to the discussion in #2118.

Copy link
Collaborator

@rickeylev rickeylev 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 this is going to work because of bzlmod visibility. rules_python only has visibility to the repos in its module.bazel, so if another repo is rendered into the generated code, it won't be visible.


I was, perhaps naively, thinking to have, quite literally, a single macro call in the generated code:

load("@rules_python//whatever.bzl", "define_pypi_package")

define_pypi_package(
  name = "foo",
  ...
)

Though admittedly, I hadn't thought through what all the args would be and if such a call wouldn't be overly complicated.

In any case, the idea behind doing it as one macro was to allow additional targets to be defined. The cases I had in mind were to define a cc_library or entry point.

A second reason for doing it this way to avoid having lots of symbols to override. Though, I suppose on advantage of naming the specific symbols to load is you can reduce how much is loaded.

Perhaps splitting the difference? Have a single macro per "type" of pypi package that gets defined. e.g., a wheel based package needs to load the define_pypi_package_from_wheel(...) symbol; an sdist-based package needs to load the define_pypi_from_sdist(...) symbol.

I'm feeling pretty -1 on the granularity in this PR, though, e.g. filegroup, py_library, etc. Those really seem like implementation details of "define a pypi package"

@aignas
Copy link
Collaborator Author

aignas commented Aug 30, 2024

I tried implementing it with bigger granularity, but then the question is what gets passed as whl and py_library deps because those could be something like

deps = ["//foo:pkg"] + select({
    "@os:linux": ["//bar:pkg"],
    "default": [],
})

And because of that reason we cannot just pass foo and bar as deps. Maybe we should see what happens if we do pass foo and bar and omit the implementation details of the selects to the rule, but that may be a step back in terms of abstractions.

@rickeylev
Copy link
Collaborator

I think I need to go look at the generated BUILD files to get a better sense of things...

Maybe step one should be refactoring so that the generated build file is simpler? See local_runtime_repo for what I mean: https://github.com/bazelbuild/rules_python/blob/main/python/private/local_runtime_repo.bzl#L32

The generated BUILD file is super simple and just calls a macro with the minimal args. You can just look at the define_local_runtime_toolchain_impl macro to see what ends up in that build file and don't have to dig out the generated build file.


I don't see why we can't pass a select()? The define_pypi_package function would be a macro. Under the hood it calls the rule and passes the deps object.

@aignas
Copy link
Collaborator Author

aignas commented Nov 2, 2024

#2347 has replaced this when it comes to better abstractions. Whereas the overriding of the targets is a separate question.

@aignas aignas closed this Nov 2, 2024
@aignas aignas deleted the refactor-render-to-macro branch November 2, 2024 04:14
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.

2 participants