chore: scripts: make go imports at least 2X faster#11695
Conversation
masih
changed the title
masih
force-pushed
the
masih/fix-go-imports-faster
branch
from
726e96a to
f6b6c73
Compare
masih
requested review from
Stebalien,
ZenGround0,
aarshkshah1992,
arajasek,
jennijuju,
magik6k,
rjan90,
rvagg and
snadrus
as code owners
Stebalien
removed request for
Stebalien,
ZenGround0,
aarshkshah1992,
arajasek,
jennijuju,
magik6k,
rjan90 and
snadrus
|
Yes I think we should go with the standard approach for imports. We don't need to be doing anything real fancy. |
snadrus
left a comment
snadrus
left a comment
There was a problem hiding this comment.
Consider the simpler goimports default layout.
yeah, I agree, I did a quick pass on this script recently to make it slightly faster and less messy by changing the I like consistency, but this script is super annoying and slows down development. |
|
Honestly, I'm fine if we just garble them up for generated files as long as the sorting is consistent. |
|
(if we only apply the import sorting to generated files, that is) |
Use native go implementation to sort and adjust imports. Compared to the previous bash version, this is at least 2X faster.
masih
force-pushed
the
masih/fix-go-imports-faster
branch
from
f6b6c73 to
ac0d623
Compare
| written, err := target.Write(replacement) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return target.Truncate(int64(written)) |
There was a problem hiding this comment.
nit: I'd generally recommend that you truncate then write. It shouldn't make any difference at the OS level, but it'll reduce the chances of someone observing a garbage version.
(well, actually, it might make a difference with a CoW filesystem...)
But this is getting deep into nit forest.
There was a problem hiding this comment.
Interesting; thank you for pointing that out
|
👌 nice, almost exactly 2x faster |
Use native go implementation to sort and adjust imports. Compared to the previous bash version, this is at least 2X faster.
Use native go implementation to sort and adjust imports. Compared to the previous bash version, this is at least 2X faster.
Lotus imports fixing was optimised to run as a go script for a faster turnaround. The changes here remove the leftover direct calls to `goimports` such that all imports are now fixed with the same script. This removes the need for installing `goimports` during CI workflows or local development environment. Relates to: * #11695
masih
changed the title
Lotus imports fixing was optimised to run as a go script for a faster turnaround. The changes here remove the leftover direct calls to `goimports` such that all imports are now fixed with the same script. This removes the need for installing `goimports` during CI workflows or local development environment. Relates to: * #11695
5 participants
Related Issues
None.
Proposed Changes
fiximportsis run up to four times across various make targets. The previous implementation used various commands to find go files, exclude irrelevant ones, etc. all in bash. It was taking ~25 seconds every time on my machine.The changes here re-implement the bash script in pure go, with 1:1 functionality. The work here uses native go implementation to sort and adjust imports, and uses
goimportsunder the hood to format imports just like the bash script. Compared to the previous bash version, this is at least 2X faster, taking ~10 seconds to run on my machine.Additional Info
The convention this repository follows is to group filecoin-project imports across 2 additional separate groups. The purpose of fixing imports is to indeed have a consistent formatting across the files. However, it is unclear why the choice of grouping is very particular. Relaxing the grouping to simply SDK and non-SDK groups can reduce the time it takes to fix them to ~5 seconds. Additionally, this means a lot of the generated files would not need import reformatting at all.
I think we should consider a simpler more straightforward import formatting. To be discussed with wider dev team.
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps