Skip to content

Comments

cmake: Make Python 2 bytecode pre-compilation optional#1356

Merged
facekapow merged 1 commit intodarlinghq:masterfrom
zhaofengli:pyc-optional
Apr 26, 2023
Merged

cmake: Make Python 2 bytecode pre-compilation optional#1356
facekapow merged 1 commit intodarlinghq:masterfrom
zhaofengli:pyc-optional

Conversation

@zhaofengli
Copy link
Contributor

This affects performance while preserving compatibility.

We can't remove it altogether just yet since there are still things depending on it, like older versions of Xcode. A new prefix has xattr and xcode-select --install depending on Python 2.

Ref: #1074

Copy link
Member

@facekapow facekapow left a comment

Choose a reason for hiding this comment

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

LGTM

In my opinion, this seems like a good solution for now; you're right that it shouldn't cause any issues, just make it execute slower. I'll wait for input from one of the other maintainers, but I certainly prefer this over including the pyc files in the repos.

Copy link
Member

@LubosD LubosD left a comment

Choose a reason for hiding this comment

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

LGTM

@bugaevc
Copy link
Member

bugaevc commented Apr 23, 2023

How about rather than making this an off-by-default option (which nobody is going to enable), we detect whether Python 2 is present and auto-enable pyc in that case?

@bugaevc
Copy link
Member

bugaevc commented Apr 23, 2023

Ah, it's on by default. Fine then, though autodetection (i.e. without REQUIRED and then if (Python2_Interpreter_FOUND)) would still be nice.

This affects performance while preserving compatibility.

Ref: darlinghq#1074
@facekapow facekapow merged commit 410e215 into darlinghq:master Apr 26, 2023
@zhaofengli zhaofengli deleted the pyc-optional branch April 26, 2023 18:57
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.

4 participants