Skip to content

Replace spaces in platform names with underscores#491

Merged
agronholm merged 1 commit into
pypa:mainfrom
tucked:platform-space
Jan 23, 2023
Merged

Replace spaces in platform names with underscores#491
agronholm merged 1 commit into
pypa:mainfrom
tucked:platform-space

Conversation

@tucked
Copy link
Copy Markdown
Contributor

@tucked tucked commented Nov 29, 2022

Without this, wheels produced on such platforms will cause a WheelError.

For example: https://gist.github.com/tucked/871d4ec2364379249bf524cf3a42f308

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 29, 2022

Codecov Report

Base: 68.35% // Head: 68.35% // No change to project coverage 👍

Coverage data is based on head (160fce6) compared to base (70d8f7f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #491   +/-   ##
=======================================
  Coverage   68.35%   68.35%           
=======================================
  Files          12       12           
  Lines         929      929           
=======================================
  Hits          635      635           
  Misses        294      294           
Impacted Files Coverage Δ
src/wheel/bdist_wheel.py 52.83% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Without this, wheels produced on such platforms will cause a WheelError.
@tucked
Copy link
Copy Markdown
Contributor Author

tucked commented Dec 14, 2022

@agronholm, would you mind taking a look at this?
(If there's someone more appropriate, please LMK!)

@FFY00
Copy link
Copy Markdown
Member

FFY00 commented Jan 23, 2023

Hum... interesting. I don't think platforms are supposed to have spaces in their names though. Where did you get your Python from?

@tucked
Copy link
Copy Markdown
Contributor Author

tucked commented Jan 23, 2023

I don't think platforms are supposed to have spaces in their names though.

What makes you say that? pip seems to expect them ((?P<plat>[^\s-]+?)).
I agree it's probably not advisable, but I'm aware of at least one real-world system that has one (isilon onefs).

Where did you get your Python from?

It's a lightly modified copy of the FreeBSD Python 3.8 port, but the platform name is not specific to Python, FWIW (e.g. uname -s or perl -e "use POSIX qw(uname); print uname();" show the same name on this platform).

@FFY00
Copy link
Copy Markdown
Member

FFY00 commented Jan 23, 2023

Well, that's true. I went over the wheel spec again and in fact I don't see anything limiting the usage spaces.

I don't think normalizing the space to underscore is the right fix, the code should be able to handle spaces in the platform tag instead.

But let's wait for the maintainers 😄

@agronholm
Copy link
Copy Markdown
Contributor

Well, that's true. I went over the wheel spec again and in fact I don't see anything limiting the usage spaces.

Maybe you missed this?

Each component of the filename is escaped by replacing runs of non-alphanumeric characters with an underscore _:

@FFY00
Copy link
Copy Markdown
Member

FFY00 commented Jan 23, 2023

Yep, thank you!

@agronholm
Copy link
Copy Markdown
Contributor

agronholm commented Jan 23, 2023

So the current version of wheel only replaces dashes with underscores, presumably because there weren't any known platform names that contained other non-alphanumeric characters. The upcoming 1.0 version will have more stringent escaping in place, but meanwhile I can certainly make a quick fix for this particular case.

@agronholm agronholm merged commit bd02d4c into pypa:main Jan 23, 2023
@agronholm
Copy link
Copy Markdown
Contributor

I didn't realize I was looking at a PR until after I wrote that previous comment 😅
This saved me some trouble. Thanks!

@tucked tucked deleted the platform-space branch January 23, 2023 21:59
@tucked
Copy link
Copy Markdown
Contributor Author

tucked commented Jan 23, 2023

😋 np, thank you!

agronholm added a commit that referenced this pull request Jan 23, 2023
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