Skip to content

fix(uv): fix evaluation of PEP 508 specs involving nested parentheses#901

Merged
jbedard merged 1 commit intoaspect-build:mainfrom
acozzette:pep508
Apr 6, 2026
Merged

fix(uv): fix evaluation of PEP 508 specs involving nested parentheses#901
jbedard merged 1 commit intoaspect-build:mainfrom
acozzette:pep508

Conversation

@acozzette
Copy link
Copy Markdown
Collaborator

This change fixes two bugs in pep508_evaluate.bzl:

  • _new_expr used _current[0] (outermost) instead of _current[-1] (innermost) when resolving the active sub-expression, causing values inside nested parens to land in the wrong expression node
  • There was a typo of current.tree._append(value) instead of current.tree.append(value).

This change also adds a Starlark unit test suite for the evaluate function.


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: no

Test plan

  • New test cases added

@aspect-workflows
Copy link
Copy Markdown

aspect-workflows Bot commented Mar 31, 2026

Bazel 8 (Test)

All tests were cache hits

112 tests (100.0%) were fully cached saving 56s.


Bazel 9 (Test)

All tests were cache hits

112 tests (100.0%) were fully cached saving 1m.


Bazel 8 (Test)

e2e

All tests were cache hits

51 tests (100.0%) were fully cached saving 42s.


Bazel 9 (Test)

e2e

All tests were cache hits

51 tests (100.0%) were fully cached saving 1m 8s.


Bazel 8 (Test)

examples/uv_pip_compile

All tests were cache hits

1 test (100.0%) was fully cached saving 444ms.

@acozzette
Copy link
Copy Markdown
Collaborator Author

@xangcastle I was wondering if you could take a look at this PR when you have a chance? I'm not sure what the CI error is about but it looks like it's not related to my change.

Comment thread uv/private/markers/pep508_evaluate.bzl
jbedard added a commit that referenced this pull request Apr 6, 2026
See issues on PRs that come from forks such as
#901

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
Copy link
Copy Markdown
Member

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

🌮

@jbedard
Copy link
Copy Markdown
Member

jbedard commented Apr 6, 2026

If you rebase the CI error should be gone now

jbedard added a commit that referenced this pull request Apr 6, 2026
See issues on PRs that come from forks such as
#901

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
jbedard added a commit that referenced this pull request Apr 6, 2026
See issues on PRs that come from forks such as
#901

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
This change fixes two bugs in pep508_evaluate.bzl:
- `_new_expr` used `_current[0]` (outermost) instead of `_current[-1]`
  (innermost) when resolving the active sub-expression, causing values
  inside nested parens to land in the wrong expression node
- There was a typo of `current.tree._append(value)` instead of
  `current.tree.append(value)`.

This change also adds a Starlark unit test suite for the evaluate function.

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
@acozzette
Copy link
Copy Markdown
Collaborator Author

If you rebase the CI error should be gone now

I gave that a try but it's still not working unfortunately. I see it's trying to fetch something involving refs/remotes/origin/pep508*, and I think that's probably failing because origin points to aspect-build/rules_py rather than my personal fork.

@jbedard jbedard merged commit d88b0f3 into aspect-build:main Apr 6, 2026
2 of 4 checks passed
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