[proxy] github.com← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public

Conversation

Copy link
Member

vstinner commented Dec 17, 2018

Fix platform.architecture() for macOS universal binaries: it no
longer uses the system's "file" command if the executable parameter
is not set.

https://bugs.python.org/issue35348

Fix platform.architecture() for macOS universal binaries: it no
longer uses the system's "file" command if the executable parameter
is not set.
vstinner changed the title bpo-35348: platform.architecture() don't use file bpo-35348: Fix platform.architecture() on macOS Dec 17, 2018
Lib/platform.py Outdated

# bpo-35348: For Python executable, don't use the file command
if executable is None or executable == sys.executable:
if sys.platform == 'win32':
Copy link
Member

Choose a reason for hiding this comment

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

Why not use _default_architecture?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that it's time to drop MS-DOS and 16-bit Windows. I don't see the point of keeping a _default_architecture = {'win32': ('', 'WindowsPE')} dictionary.

Copy link
Member

Choose a reason for hiding this comment

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

Using _default_architecture can make supporting new non-standard architectures easier.

But if you think than it is unlikely that new special cases will be added in future, perhaps remove _default_architecture?

Copy link
Member

Choose a reason for hiding this comment

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

Please ignore the last sentence. I missed that the definition of _default_architecture was removed in the original commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a new commit "Add platform._DEFAULT_LINKAGE". I dislike _default_architecture: it was documented whereas it's a private attribute, it's lower-case whereas PEP 8 suggests upper-case for constants, and it requires to check if bits or linkage are empty.

vstinner requested a review from ned-deily December 17, 2018 09:41
Copy link
Member

Technically LGTM, but I do not know whether the behavior change is desirable.

Copy link
Member Author

Technically LGTM, but I do not know whether the behavior change is desirable.

Me neither :-) I would like to get a review of a macOS expert. It seems like @ronaldoussoren dislike the change:
https://bugs.python.org/issue35348#msg331959

Copy link
Member Author

@ronaldoussoren dislike this approach:
https://bugs.python.org/issue35348#msg331959

@malemburg also seems to dislike this change:
https://bugs.python.org/issue35348#msg332016

So I abandon this change.

vstinner closed this Dec 18, 2018
vstinner deleted the platform_architecture branch December 18, 2018 11:11
Copy link
Member Author

See PR #11208 which update the doc instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants