Conversation
lmmx
force-pushed
the
rust-language-support
branch
from
9c06a6d to
b306063
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #989 +/- ##
==========================================
+ Coverage 89.66% 89.70% +0.03%
==========================================
Files 74 77 +3
Lines 13761 14599 +838
==========================================
+ Hits 12339 13096 +757
- Misses 1422 1503 +81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📦 Cargo Bloat ComparisonBinary size change: +1.23% (16.2 MiB → 16.4 MiB) Expand for cargo-bloat outputHead Branch ResultsBase Branch Results |
This comment was marked as outdated.
This comment was marked as outdated.
lmmx
force-pushed
the
rust-language-support
branch
2 times, most recently
from
ffd11de to
beab6c1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
lmmx
force-pushed
the
rust-language-support
branch
from
e74a5a9 to
a5b5261
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
So we are rolling, I included a very simple demo to run it in a Docker container too in #989 if anyone wants to test in particular scenarios of interest The first thing that comes to mind for me is that the error message when you don't have rustup on your machine is not very helpful. The CI has rustup pre-installed, if you don't have it you now get: Perhaps we could give a hint (hey, you might want to go get rustup)? Maybe drop a link to our/their docs? |
|
Now we get a little further by using the installed rustup binary path instead of relying on PATH. When rustup is downloaded and installed under Instead we get an issue that rustup was not configured, i.e. we need to set the |
|
Trace in the container shows still not managing to set the rust version specifier (still unfilled at 0.0.0)
Click to show full trace
|
|
Progress! Installation now queries version for cached toolchains
Version is being queried:
Installation succeeds but then execution still fails:
then no more trace but hook fail in main output
Click to show full trace
|
|
aaand success in the container 🎉 I needed to append both prek/src/languages/golang/golang.rs Line 137 in 59b82c7 |
|
I expected that container success to mean the CI
This (61298df) now fixes that CI failure where system rustup caused us to skip installation but then the local binary didn't exist for
|
lmmx
force-pushed
the
rust-language-support
branch
from
e5b4265 to
a40c2f1
Compare
|
insta is being difficult 🤨 - rustc 1.X
+ rustc 1.X |
|
phew, final test failure to solve (this one can be repro'd locally in regular tests) |
lmmx
force-pushed
the
rust-language-support
branch
from
2785b5b to
59f9f52
Compare
# Link checker using local hooks with language: rust
# Pre-commit will automatically install lychee via cargo
- repo: local
hooks:
- id: lychee
name: lychee (local links only)
entry: lychee
language: rust
additional_dependencies: ["cli:lychee:0.20.1"]
files: \.(md|rst)$
exclude: ^web/website/themes/
args:
- --no-progress
# For PRs: whitelist PRQL domains and local files only
# Pattern matches:
# - github.com/PRQL/* and github.com/prql/*
# - prql-lang.org/*
# - raw.githubusercontent.com/PRQL/* and raw.githubusercontent.com/prql/*
# - file://* (local/relative links)
# All other external links are skipped for faster PR checks
- --include=^(https://(github\.com/(PRQL|prql)|prql-lang\.org|raw\.githubusercontent\.com/(PRQL|prql))|file://)
- id: lychee-all
name: lychee-all (all links)
entry: lychee
language: rust
additional_dependencies: ["cli:lychee:0.20.1"]
stages: [manual]
files: \.(md|rst)$
exclude: ^web/website/themes/
args: ["--config=.config/lychee.toml", "--no-progress"] |
|
ok it looks it does work with the additional deps, phew 🎉 (No speed difference) Putting this back as draft and will amend that test later |
|
That should do it... 5e543d5 uses a This would get much faster with cargo binstall. That's a priority now I think (ongoing PR for this in pre-commit) edit it says the operation was cancelled (it seems to have timed out after 4m29s spent 
Unsure if this is the Lua service unavailable or the timeout after "10m0s"... which would be the Windows run... maybe a transient CI failure? |
|
Arg, going to have to find other cli libs, it looks like it's too slow... The jobs are all timing out at their limit. (I have to head out now, will complete later) I'd instead switch to a no-dep very small CLI tool (one with just plain I'm leaving a request for testers in the Astral Discord where people showed interest in this! cargo install --git https://github.com/lmmx/prek --branch rust-language-support
|
|
Thanks for all the hard work! We can create a minimal test repo for Rust at https://github.com/orgs/prek-test-repos/repositories - I sent you an invitation to the org. For #681, it’d be better to handle that in another PR since this one’s getting pretty big already. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
For pytest-mimic (the PR with the updated action-validator hook rev), running hooks is now 3x faster with prek than with pre-commit 😎 cloudstack takes a long time to run (9s) 😴 that is still 3x faster than with pre-commit (29s). Life's too short to hyperfine that (would take mins+). robustmq exitted with non-zero exit code on the first run, so hyperfine didn't want to benchmark it. Not entirely sure what was going wrong there (clang is erroring out) but pre-commit does the same. prek takes 5.6s and pre-commit takes 5.9s - so some speedup. About 5% faster. shiro runs 25% faster with prek 🎉 |
|
I was wondering if prek itself could now use a Rust hook, for the typos linter. It doesn't look like that If I swap the typos hook to That doesn't match what pre-commit gives, but pre-commit does error too: Deleting the caches of both prek/pre-commit did not heal it so I assume it isn't functional. |
lmmx
force-pushed
the
rust-language-support
branch
from
875e25e to
536fd59
Compare
|
I also wanted to test on some of these 'rust for web' repos, e.g. radix The initial hook repo clone setup for radix with pre-commit takes an inordinately long time: 3m30s Unfortunately something is amiss with prek in the Rust hook support here and it is erroring out the issue here is cargo-deny has a root level Cargo.toml that is both a workspace and a package, and the
hook installation
Not significantly faster, but consistent! 👍 after installation hyperfine -n prek 'prek run -a' -n pre-commit 'pre-commit run -a' -i
The time to run is exactly the same 🙃 |
lmmx
force-pushed
the
rust-language-support
branch
from
3e1b4b9 to
e37d3d8
Compare
|
OK I think this is feature complete and happy to merge now. We have basic remote hooks in our repo for testing, the caching works, and I've tested with several repos and the only ones where it didn't work it also didn't work with pre-commit. To cover the various ways that repos can signal they contain packages I've added unit tests (workspace cargo toml, workspace cargo toml with packages, etc) but I don't think these need dedicated remote repo hook tests. I think this is ready! Further developmentFollow up PRs:
|
j178
force-pushed
the
rust-language-support
branch
from
71d3cf7 to
02cb9d5
Compare
lmmx
force-pushed
the
rust-language-support
branch
from
46d0fda to
9e3547d
Compare
|
Thanks @lmmx! I think this is good to merge now. Here are a few TODOs I might want to tackle next:
|
|
Great stuff! I had a go at these extra goals and they ain't as easy as it looks to get right. Attempted here if it's any use for future reference but getting nothing but test failures 👎 https://github.com/lmmx/prek/tree/rust-support-polish |
3 participants
feat(wip): implement run with filesystem path extraction [not correct]Container testing
Click to show instructions/notes
Then in container:
This tests prek's ability to download and install Rust from scratch with a toolchain name (rather than explicit version number).