Lando python auto-formatting doesn't match what lint expects
Categories
(Conduit :: Lando, defect)
Tracking
(Not tracked)
People
(Reporter: glandium, Assigned: zeid)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
When D121944 landed, it triggered a black lint error, which was subsequently fixed by sheriffs.
The changes that landing did are: https://phabricator.services.mozilla.com/D121944?vs=468351&id=468355#toc
The changes that sheriffs undid are: https://hg.mozilla.org/integration/autoland/rev/a8d4d6248454
Locally, both the latest version of black and the version of black used by CI were happy with the patch before landing.
|
Reporter | |
Comment 1•3 years ago
|
||
|
||
Comment 2•3 years ago
|
||
Zeid, do you know what is going on? Different version of black?
merci
|
Assignee | |
Comment 3•3 years ago
|
||
|
|
Assignee | |
Comment 4•3 years ago
|
||
@sylvestre -- yes indeed this looks related to the black version. We have a patch that fixes this, we'll aim to land it asap.
|
|
||
Comment 5•3 years ago
|
||
What about using
https://searchfox.org/mozilla-central/source/tools/lint/python/black_requirements.in / https://searchfox.org/mozilla-central/source/tools/lint/python/black_requirements.txt
directly ?
This is going to be painful to maintain otherwise
|
|
Assignee | |
Comment 6•3 years ago
|
||
|
|
Assignee | |
Comment 7•3 years ago
|
||
(In reply to Sylvestre Ledru [:Sylvestre] from comment #5)
What about using
https://searchfox.org/mozilla-central/source/tools/lint/python/black_requirements.in / https://searchfox.org/mozilla-central/source/tools/lint/python/black_requirements.txt
directly ?This is going to be painful to maintain otherwise
Yes agreed -- the plan is to have a separate service or image that handles the formatting that does not rely on the Lando image. That will be replacing this way of running black in the future.
|
|
Assignee | |
Comment 8•3 years ago
|
||
Lando is now running the updated version of Black and this particular issue is resolved. We are going to be revisiting the implementation of where the code formatters live (currently they are in the lando docker image, which is not ideal since we may need different versions and different formatters). :sheehan is there a bug tracking this already?
|
|
Reporter | |
Comment 9•3 years ago
|
||
Lando is now running the updated version of Black and this particular issue is resolved.
I beg to differ. Lando is now using 21.5b2 and central uses 20.8b1. There is still room for this to happen.
|
|
Assignee | |
Comment 10•3 years ago
|
||
Yes that's correct, there is a version discrepancy, but the formatting issue that was encountered in this bug should not occur as the two versions should format the file the same way. There may be other inconsistencies but we will have to solve this problem in an alternate way in order to match the version. We're basically piggy backing off of the package that's used by Lando right now. The better solution would be to separate Lando's dependency from the formatter dependency, and as mentioned this is planned.
:glandium if you are running into a formatting issue right now with this current version, let me know and we can revisit this in the immediate term, otherwise let's wait until we have a better solution for version matching/pinning.
|
|
Assignee | |
Comment 11•3 years ago
|
||
:sheehan was there a specific reason we chose this exact version of black? Perhaps you can shed some light as well here.
|
|
Reporter | |
Comment 12•3 years ago
|
||
Here's a try with black 21.5b2: https://treeherder.mozilla.org/logviewer?job_id=348138013&repo=try&lineNumber=122
All the files reported there will have the issue reported here if they're modified by a patch and land with lando.
|
|
Assignee | |
Comment 13•3 years ago
|
||
Ah -- ok, I will update the version to match manually for now, until we have a long term solution for this.
|
|
Reporter | |
Comment 14•3 years ago
|
||
Actually, reformatting them with black 21.5b2 first and linting with black 20.8b1 doesn't yield errors.
|
||
Comment 15•3 years ago
|
||
(In reply to Zeid Zabaneh [:zeid] from comment #8)
Lando is now running the updated version of Black and this particular issue is resolved. We are going to be revisiting the implementation of where the code formatters live (currently they are in the lando docker image, which is not ideal since we may need different versions and different formatters). :sheehan is there a bug tracking this already?
I thought there was but I could not find it - filed bug 1725247.
(In reply to Zeid Zabaneh [:zeid] from comment #11)
:sheehan was there a specific reason we chose this exact version of black? Perhaps you can shed some light as well here.
Hmmm I was under the impression we ran the latest black, but it seems that was wrong. We could either revert Lando's black to v20 or bump the in-tree version to v21. This will be fixed once we address bug 1725247.
|
|
Assignee | |
Comment 16•3 years ago
|
||
|
|
Assignee | |
Comment 17•3 years ago
|
||
This should now be fixed, please let me know if the issue is persisting for any reason.
Description
•