-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
GH-113464: Add a warning when building the JIT #118481
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.
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
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Though, I do wonder if it's worth adding commit rules or similar to catch formatting issues? Would be nice to just have this tackled automagically?
Agreed. There's already a |
They can be added in We can add Black to pre-commit (after the Ruff config) like: - repo: https://github.com/psf/black-pre-commit-mirror
rev: 24.4.2
hooks:
- id: black
files: TODO We can run pylint via the existing Ruff by selecting the PL rules in a local (cc @AlexWaygood FYI)
(Yeah, I usually only run pylint occasionally and don't in CI because it's quite noisy, but if you've resolved them and want to maintain that, I recommend doing it this way.) |
@brandtbucher If you think that'd be nice, I can take a look at adding a job to do this in |
That would be great, yeah. I'm not picky about the set of rules itself, so if the defaults pass then we can just use those. |
This banner is printed even when using |
Hm. This script should only be part of the Makefile if we're actually building the JIT. It looks like So we should update |
Yeah, I'm on it. |
Thanks! |
PEP 744 makes a bunch of promises once "the JIT builds successfully without displaying warnings to the user". We don't currently do that... so we should.
This PR adds a small banner during JIT builds reminding the user that JIT support for their platform is still experimental, and asking them to report any bugs. This banner is on by default, but can be disabled per-target as they become stable.
This PR also includes a few mostly-cosmetic changes to appease
mypy
,black
, andpylint
. They're in separate commits, so they're easier to review.