fix(uv): add $(EXECROOT) make-var to anchor native sdist env paths across the backend chdir#1233
Open
sallustfire wants to merge 5 commits into
Open
Conversation
…paths
Builds a native sdist whose C extension consumes an in-repo cc_library via
`override_package(env=/toolchains=)`-shaped make-vars: CPPFLAGS = `-I$(DEP_INC)`
and LDFLAGS = `$(DEP_LIB_A)`. Both expand to Bazel workspace-relative paths,
which resolve from the action execroot but not from the PEP 517 backend's cwd
inside the unpacked sdist worktree.
This commit is intentionally RED — it documents the bug. The compile fails with:
mod.c: fatal error: 'dep.h' file not found
cc ... -Iuv/private/pep517_whl/tests/native_dep -c mod.c
The fix follows.
…end chdir
`override_package(env = {...})` values expanded from make-vars are Bazel
workspace-relative; they resolve from the action execroot but not from the
PEP 517 backend's cwd, which `python -m build` changes into the unpacked sdist
worktree. `$(EXECROOT)` expands to a marker that build_helper replaces with the
absolute execroot at build time, so `-I$(EXECROOT)/$(DEP_INC)` is anchored
deterministically — no path detection, no per-var-name list, and it applies
uniformly to any env var (e.g. JAVA_HOME).
Fixes the regression from the previous commit.
py_binary startup benchmark
sys.path quality
✅ No regression detected (PR is +0.8% vs HEAD main) |
Adds the bzl_library target gazelle generates for defs.bzl, keeping the BUILD file consistent with the repo's gazelle check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
|
Hi, could you resolve the conflicts? |
f719140 to
352f05b
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
pep517_native_whl(viauv.override_package(env = {...}, toolchains = [...])) expands$(VAR)make-variables in the build action'senvfrom the toolchains it depends on. Those values are workspace-relative paths: a dependency's include dir expands to something likebazel-out/.../include, which resolves from the execroot but not from the PEP 517 backend's cwd.python -m buildruns the backend with its cwd inside the unpacked sdist worktree (build_helperinvokes it withcwd=<worktree>), so a workspace-relative-Ibazel-out/.../includedangles and the build fails withfatal error: '<header>' file not found, or the link can't find the dep archive.Add a
$(EXECROOT)make-variable to anchor those paths, e.g.-I$(EXECROOT)/$(DEP_INC). The execroot isn't known at analysis time, so$(EXECROOT)expands to a marker thatbuild_helperreplaces with the absolute execroot at build time, before the chdir. It's a plain text substitution rather than flag parsing, so it works for any env var and any flag form (-I,-L,-Wl,-rpath,-framework,CC, ...).EXECROOTis reserved for this. If a toolchain intoolchainsalready exports it,$(EXECROOT)would expand to that value instead of the marker and silently skip anchoring, so analysis now fails with a message pointing at the collision rather than producing a confusing compile error later.I went with an opt-in anchor rather than having
build_helperdetect and absolutize path-bearing flags itself: it can't reliably tell an execroot-relative path from a symbol/framework name or a worktree-relative path, can't cover every flag syntax, and would risk mangling values it doesn't need to touch.More generally, what might be missing is some notion of a
depsanalog tocargo_build_scriptthat can inspect a CcInfo provider and get theCcInfo.linking_context. This is a much bigger design question if were to be offered first-class since setuptools handles the linking, and this would all have to get flattened into LDFLAGS/extra_link_args.Regression test
Adds
uv/private/pep517_whl/tests/native_dep: acc_librarywhose include dir and static archive are surfaced as make-vars (standing in foroverride_package(toolchains=, env=)), consumed by a small PEP 517 backend that compiles a C extension against them.Split into a red/green pair. The test commit wires the env as plain
-I$(DEP_INC)/$(DEP_LIB_A), which fails today because the backend compiles with an execroot-relative-Ithat dangles after the chdir. The fix commit adds$(EXECROOT)and switches the test to-I$(EXECROOT)/$(DEP_INC).Changes are visible to end-users: yes
$(EXECROOT)documented on theoverride_package(env=)tag and thepep517_native_whlenvattr)Testing
bazel test //uv/private/pep517_whl/tests/native_dep:native_dep_env_testbuildifier --mode=check uv/private/pep517_whl/rule.bzl