Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix issues in discovering ruff in pip build environments #13881

Merged
merged 3 commits into from
Oct 29, 2024

Conversation

jsurany
Copy link
Contributor

@jsurany jsurany commented Oct 22, 2024

Summary

Changes in this PR #13591 did not allow correct discovery in pip build environments.

# both of these variables are tuple[str, str] (length is 2)
first, second = os.path.split(paths[0]), os.path.split(paths[1])

# so these length checks are guaranteed to fail even for build environment folders
if (
    len(first) >= 3
    and len(second) >= 3 
    ...
)

Here we instead use pathlib, and we check all pip-build-env- paths for the folder that is expected to contain the ruff executable.

Here we update the logic to more properly split out the path components that we use for pip-build-env- inspection.

Test Plan

I've checked this manually against a workflow that was failing, I'm not sure what to do for real tests. The same issues apply as with the previous PR.

Copy link
Contributor

github-actions bot commented Oct 22, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Needs some work IMHO.

python/ruff/__main__.py Outdated Show resolved Hide resolved
python/ruff/__main__.py Outdated Show resolved Hide resolved
python/ruff/__main__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

What was the actual problem here?

@gaborbernat
Copy link
Contributor

gaborbernat commented Oct 28, 2024

What was the actual problem here?

#13591 does not work 😆 this fixes that. @jsurany is a colleague of mine and when validating that release discovered this issue.

@charliermarsh charliermarsh enabled auto-merge (squash) October 29, 2024 15:46
@charliermarsh charliermarsh merged commit 60a2dc5 into astral-sh:main Oct 29, 2024
17 checks passed
@jsurany jsurany deleted the pip_build_envs branch October 29, 2024 15:51
@dhruvmanila dhruvmanila added the bug Something isn't working label Oct 30, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants