Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

fix: allow spaces in whl_librarys #2334

Merged
merged 25 commits into from
Oct 30, 2024
Merged

fix: allow spaces in whl_librarys #2334

merged 25 commits into from
Oct 30, 2024

Conversation

mark-thm
Copy link
Contributor

Fixes #617.

Modern setuptools versions contain files critical to setuptools functionality loaded proactively on module load that contain spaces. Bazel 7.4.0+ now supports files with spaces in their names.

rickeylev
rickeylev requested changes Oct 23, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
python/private/glob_excludes.bzl Outdated Show resolved Hide resolved
@aignas
Copy link
Collaborator

aignas commented Oct 27, 2024

FYI, it would be better to wait for #2347 so that testing here is easier.

@aignas aignas mentioned this pull request Oct 28, 2024
Merged
aignas
aignas reviewed Oct 29, 2024
View reviewed changes
Copy link
Collaborator

@aignas aignas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Do we need the same excludes in whl_library_targets.bzl file?

python/glob_excludes.bzl Outdated Show resolved Hide resolved
@rickeylev
Copy link
Collaborator

Here's an easy way to detect 7.4: hasattr(native, "legacy_globals")

@rickeylev
Copy link
Collaborator

To clarify: put that in python/private/utils.bzl, near the bottom you'll see various IS_BAZEL_BLABLA constants

rickeylev
rickeylev reviewed Oct 29, 2024
View reviewed changes
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 pushed some changes to address review comments and avoid the issue with workspace+bazel_features

python/glob_excludes.bzl Outdated Show resolved Hide resolved
python/private/py_repositories.bzl Outdated Show resolved Hide resolved
@mark-thm
Copy link
Contributor Author

@rickeylev thanks, I hadn’t had time to pick this up with your suggestion

@rickeylev rickeylev added this pull request to the merge queue Oct 30, 2024
Merged via the queue into bazelbuild:main with commit f40038e Oct 30, 2024
4 checks passed
aignas
aignas reviewed Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

LGTM

@mark-thm mark-thm deleted the me/spaces branch October 30, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickeylev rickeylev rickeylev approved these changes

@aignas aignas aignas left review comments

@groodt groodt Awaiting requested review from groodt groodt is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Heads up: setuptools contain files with whitespaces in their names
3 participants
@mark-thm @aignas @rickeylev