feat(bzlmod): cross-platform builds without experimental_index_url#2325
feat(bzlmod): cross-platform builds without experimental_index_url#2325aignas merged 42 commits intobazel-contrib:mainfrom
Conversation
… platform is known
|
@keith, could you please check to see if this is fixing the issues you were seeing previously? The CI is not fully green, but the examples and unit tests are passing, so I think this might be ready for a spin. |
| "repo": "pypi_315", | ||
| "requirement": "simple==0.0.1 --hash=sha256:deadbeef", | ||
| }, | ||
| }) |
There was a problem hiding this comment.
I am happy that my selection of the mock ctx.os values was already validating the bug in question.
|
it's possible it's just fighting getting the lockfile updated, but with this i see: looks like i hit that with or without experimental_index_url |
CHANGELOG.md
Outdated
| file irrespective if `experimental_index_url` is set by any module or not. | ||
| Fixes [#2268](https://github.com/bazelbuild/rules_python/issues/2268). A known | ||
| issue is that it may break `bazel query` and in these usecases it is advisable | ||
| to use `cquery` until we have `sdist` cross-building from source fully working. |
There was a problem hiding this comment.
download_only=True is also an option, right? Assuming one is OK with not having sdists
There was a problem hiding this comment.
It could be, but I haven't tested with regular requirements files, let me add a unit test to see how that would get wired through.
EDIT: With a few extra modifications, I think we can actually support that. This means that we can resolve #260 and have the stabilization for experimental_index_url done as a separate ticket. I've added extra docs on this, please check. :)
@keith, I should have explained better in the PR message/changelog, that with this change you will have multiple |
Co-authored-by: Richard Levasseur <richardlev@gmail.com>
We won't have correct dependencies when parsing the METADATA files because we need to pass the target platforms there. I think I got too happy too soon about the possibility of having correct builds with just 'download_only = True'.
This reverts commit bf4f644.
…ownload_only case
cc_library is OK with empty srcs |
This adds back the internal extension to ensure that users are not forced into the feature.
…tries host independent
…file entries host independent
…n lock file entries host independent
|
OK, quick status update - once #2369 is merged, I will update this PR to be |
…repos (#2369) Before this change, it was impossible for users to use the targets created with `additive_build_content` whl annotation unless they relied on the implementation detail of the naming of the spoke repositories and had `use_repo` statements in their `MODULE.bazel` files importing the spoke repos. With #2325 in the works, users will have to change their `use_repo` statements, which is going to be disruptive. In order to offer them an alternative for not relying on the names of the spokes, there has to be a way to expose the extra targets created and this PR implements a method. Incidentally, the same would have happened if we wanted to stabilize the #260 work and mark `experimental_index_url` as non-experimental anymore. I was hoping we could autodetect them by parsing the build content ourselves in the `pip` extension, but it turned out to be extremely tricky and I figured that it was better to have an API rather than not have it. Whilst at it, also relax the naming requirements for the `whl_modifications` attribute. Fixes #2187
|
@keith, could you please check this PR again? I think it should work for your usecase now. |
|
Yes sorry I'm OOO but I will as soon as I'm back next week |
|
Since the behaviour is opt-in, I'll merge the PR now and we can make further fixes later, but I am still looking forward to feedback next week. |
|
I think I could probably make whl_filegroup work, but it definitely breaks some use cases we're using today. For example today in my _LIBS = [
lib
for lib in glob(
[
"site-packages/torch/lib/*.so*",
"site-packages/torch.libs/*.so*",
"site-packages/torch/lib/*.dylib",
],
allow_empty = True,
exclude = [
"site-packages/torch/lib/*libgomp*", # Handled below
# Intel only, seem to be unnecessary for our use case
"**/libtorchbind_test*",
"**/libbackend_with_compiler*",
"**/libjitbackend_test*",
],
)
]And then I expand multiple targets based on the |
|
seems like one option is this: which pretty much gets me back to what i had before, is that one not recommended? |
With this change we finally generate the same lock file within the
legacy code
pip.parsecode path and it allows to slowly transition tousing the new code path as much as possible without user doing anything.
This moves the selection of the host-compatible lock file from the
extension evaluation to the build phase - note, we will generate extra
repositories here which might not work on the host platform, however, if
the users are consuming the
whl_libraryrepos through the hub repoonly, then everything should work. A known issue is that it may break
bazel queryand in these usecases it is advisable to usecqueryuntil we have
sdistcross-building from source fully working.Summary:
render_pkg_aliasesfor when filename is not knownbut platform is known
get_whl_flag_versionsnow generates extra args for the rulesplatform
download_only = Trueinlegacy setups
Note, that users depending on the naming of the whl libraries will need
to start using
extra_hub_aliasesattribute instead to keep theirsetups not relying on this implementation detail.
Fixes #2268
Work towards #260