-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Added Pre-Commit hook support #541
Conversation
Nice work! I had this on my todo list for half a year, decided yesterday to address it and found that you just implemented this last week. What a coincindence! |
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.
Trying to run this hook via pre-commit try-repo https://github.com/Jarmos-san/selene selene
fails with
===============================================================================
Using config:
===============================================================================
repos:
- repo: https://github.com/Jarmos-san/selene
rev: bd8b81cd7ee57727e4bd128a80bbdb1acc83a17f
hooks:
- id: selene
===============================================================================
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /tmp/tmpcir77ixa/patch1690702419-3175.
[INFO] Initializing environment for https://github.com/Jarmos-san/selene.
[INFO] Installing environment for https://github.com/Jarmos-san/selene.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Restored changes from /tmp/tmpcir77ixa/patch1690702419-3175.
An unexpected error has occurred: CalledProcessError: command: ('/tmp/tmpcir77ixa/repovkkeuvf0/rustenv-default/bin/cargo', 'install', '--bins', '--root', '/tmp/tmpcir77ixa/repovkkeuvf0/rustenv-default', '--path', '.')
return code: 101
stdout: (none)
stderr:
info: syncing channel updates for 'stable-x86_64-unknown-linux-gnu'
info: latest update on 2023-07-13, rust version 1.71.0 (8ede3aae2 2023-07-12)
info: downloading component 'cargo'
info: downloading component 'clippy'
info: downloading component 'rust-docs'
info: downloading component 'rust-std'
info: downloading component 'rustc'
info: downloading component 'rustfmt'
info: installing component 'cargo'
info: installing component 'clippy'
info: installing component 'rust-docs'
info: installing component 'rust-std'
info: installing component 'rustc'
info: installing component 'rustfmt'
error: found a virtual manifest at `/tmp/tmpcir77ixa/repovkkeuvf0/Cargo.toml` instead of a package manifest
Check the log at /home/bmr/.cache/pre-commit/pre-commit.log
I suspect Cargo expects something different?
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.
Hey thanks for reporting the issue! I did some research and stumbled across rust-lang/cargo#7599 which is why Cargo fails to correctly identify which Cargo.toml
to use based on the correct workspace! I think it might be possible to circumvent this issue by specifying the exact path to the correct workspace using the Specifying args
option of the .pre-commit-hooks.yaml
file (see related docs)--path selene
with the args
key does not work as I expect it to. I'm kinda out of ideas now since I'm not very familiar with cargo
atm.
Besides, I also noticed the other Docker based hook is breaking too! See the error logs I stumbled across:
===============================================================================
Using config:
===============================================================================
repos:
- repo: https://github.com/Jarmos-san/selene
rev: bd8b81cd7ee57727e4bd128a80bbdb1acc83a17f
hooks:
- id: selene-docker
===============================================================================
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /tmp/tmp5m_0u5m4/patch1690706713-30953.
[INFO] Initializing environment for https://github.com/Jarmos-san/selene.
[INFO] Installing environment for https://github.com/Jarmos-san/selene.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Restored changes from /tmp/tmp5m_0u5m4/patch1690706713-30953.
An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/docker', 'build', '--tag', 'pre-commit-75061517ae24f03e43af03f990801e33', '--label', 'PRE_COMMIT', '--pull', '.')
return code: 1
stdout: (none)
stderr:
#0 building with "default" instance using docker driver
#1 [internal] load .dockerignore
#1 transferring context: 2B done
#1 DONE 0.8s
#2 [internal] load build definition from Dockerfile
#2 transferring dockerfile: 1.24kB 0.1s done
#2 DONE 0.8s
#3 [internal] load metadata for docker.io/library/rust:1.64-alpine3.14
#3 ERROR: docker.io/library/rust:1.64-alpine3.14: not found
#4 [internal] load metadata for docker.io/library/bash:latest
#4 CANCELED
------
> [internal] load metadata for docker.io/library/rust:1.64-alpine3.14:
------
Dockerfile:17
--------------------
15 | cargo install --branch main --git https://github.com/Kampfkarren/selene selene
16 |
17 | >>> FROM rust:1.64-alpine3.14 AS selene-light-musl-builder
18 | RUN apk add g++ && \
19 | cargo install --no-default-features --branch main --git https://github.com/Kampfkarren/selene selene
--------------------
ERROR: failed to solve: rust:1.64-alpine3.14: docker.io/library/rust:1.64-alpine3.14: not found
Check the log at /home/space/.cache/pre-commit/pre-commit.log
Is it because perhaps the I tried building the Image based on the rust:1.64-alpine3.14
Image does not exists?Dockerfile
and the build breaks so I'm sure its because the rust:1.64-alpine3.14
wasn't found.
It might be a better idea to open another PR with a fix for the Docker build no?
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.
I agree, the Docker build should be fixed in a separate PR.
Regarding the Cargo error, I'm also not familiar with Cargo. Maybe @Kampfkarren has an idea? pre-commit's installation procedure is documented here.
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.
Would you like to take the initiative to fix the Dockerfile
since you already had the idea work on this PR earlier?
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.
I would, if I'd be familiar with containers...
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.
Oh haha...cheers then! Don't worry I'll send another with a fix next weekend then. By then I hope Kamp will help us out figure out how to fix the cargo
error.
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.
It looks like we can make it use rust-alpine
, right? I always want latest Rust anyway.
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.
Yes, it works with rust:alpine
; checked it recently.
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.
Oh great glad rust:alpine
works, so @f1rstlady do you want to try sharing a PR with a fixed Dockerfile
? I can mark this PR a draft till you have shared a mergeable PR. What do you say?
Or if you want I can look into the current Dockerfile
and see if I can fix the broken build.
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.
Here it is: #549
I haven't tested the corresponding pre-commit hook yet since I used podman instead of docker to build the repaired image.
Apologies for the delay, been very busy. @f1rstlady @Jarmos-san Yes the Docker build is broken...it was submitted by someone else and has not been properly maintained. |
Any progress on this? If this needs some work, I can try to get it done |
Well I wanted to work on it but couldn't find time but please feel free to take it up. I would be equally pleased to have someone implement the Pre-Commit hook other than me. |
- id: selene-docker | ||
name: selene (docker) | ||
description: An opinionated Lua code linter | ||
entry: selene |
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.
entry: selene | |
entry: /selene |
This should fix the Docker run.
I looked into the Any recommendations: @Jarmos-san @Kampfkarren @f1rstlady? @Jarmos-san If you could provide with me access, I would like to keep this PR open, just to build a continuity for the discussion. Or, I can open a fresh PR? Let me know what would be suitable for you. |
@amitds1997 you can reopen a new PR and I will not have no issues with it as long as I can run |
@Jarmos-san may you merge the main to get the updated docker image? |
@f1rstlady do you want me to update this PR's branch with the latest updates from the |
Ok, now I get your idea! |
Created #565 for this. |
Fixes #539
This PR adds the pre-commit hooks configurations and documentation changes to reflect its support.