Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added Pre-Commit hook support #541

Closed
wants to merge 4 commits into from
Closed

Added Pre-Commit hook support #541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b130ba
Update the docs about the `pre-commit` hook
Jarmos-san Jul 24, 2023
f8501e2
Add the `pre-commit` hook configurations
Jarmos-san Jul 24, 2023
bd8b81c
fix the pre-commit hook config filename
Jarmos-san Jul 30, 2023
f0af99e
Merge branch 'main' into main
Jarmos-san Aug 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .pre-commit-hooks.yaml
View file
Open in desktop
Copy link
Contributor

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?

Copy link
Author

@Jarmos-san Jarmos-san Jul 30, 2023 •
edited
Loading

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 args option of the .pre-commit-hooks.yaml file (see related docs) Specifying --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 rust:1.64-alpine3.14 Image does not exists? I tried building the Image based on the 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?

Copy link
Contributor

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.

Copy link
Author

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?

Copy link
Contributor

@f1rstlady f1rstlady Jul 30, 2023 •
edited
Loading

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...

Copy link
Author

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.

Copy link
Owner

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.

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

@f1rstlady f1rstlady Aug 27, 2023 •
edited
Loading

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
- id: selene
name: selene (cargo)
description: An opinionated Lua code linter
entry: selene
language: rust
types:
- lua

- id: selene-system
name: selene (system)
description: An opinionated Lua code linter
entry: selene
language: system
types:
- lua

- id: selene-docker
name: selene (docker)
description: An opinionated Lua code linter
entry: selene
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
entry: selene
entry: /selene

This should fix the Docker run.

language: docker
types:
- lua
3 changes: 2 additions & 1 deletion docs/src/SUMMARY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- [Configuration](./usage/configuration.md)
- [Filtering](./usage/filtering.md)
- [Standard Library Format](./usage/std.md)
- [Git Hook](./usage/git-hook.md)
- [Roblox Guide](./roblox.md)
- [Contributing](./contributing.md)
- [Lints](./lints/index.md)
Expand Down Expand Up @@ -41,4 +42,4 @@
- [unscoped_variables](./lints/unscoped_variables.md)
- [unused_variable](./lints/unused_variable.md)
- [Archive](./archive/index.md)
- [TOML Standard Library Format](./archive/std_v1.md)
- [TOML Standard Library Format](./archive/std_v1.md)
25 changes: 25 additions & 0 deletions docs/src/usage/git-hook.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Git Hook Support

Selene has [githooks](https://git-scm.com/docs/githooks) support thanks to
[pre-commit](https://pre-commit.com) which means you will need to add a
configuration file - `.pre-commit-config.yaml` to your repository. Thereafter
you'll need to install the hooks and `selene` will be run against all recently
changed files before you commit the changes.

This section of the document details the ways you could setup `pre-commit` for
linting your Lua code before each commits.

In the `.pre-commit-config.yaml` file add the following configurations and then
run `pre-commit install --install-hooks` to setup the pre-commit hooks.

```yaml
repos:
- repo: https://github.com/Kampfkarren/selene
rev: '' # Add the latest version of Selene here
hooks:
- selene # this will use Cargo to compile the binary before usage
- selene-system # this will use the installed binary on the system
- selene-docker # this will build a Docker image before usage
```

To ensure `pre-commit` is the latest tagged version of `selene`, run the `pre-commit autoupdate` command.