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.

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

chore: setup tooling #2

Merged
merged 26 commits into from
Jul 9, 2024
Merged

chore: setup tooling #2

merged 26 commits into from
Jul 9, 2024

Conversation

sripwoud
Copy link
Member

@sripwoud sripwoud commented Jul 1, 2024

Closes #1

  • Covers the tools mentioned in Define a basic toolset for this repository #1 (comment).
  • Defines a .setup.sh script that will install tools that are used for local development, namely cargo-nextest and dprint and convco (cocogitto does not have an interactive CLI afaict). This script and deps were picked with the following requirements in mind:
    • portability: everything should work only with the assumption that the dev has a cargo/rust environment setup.
    • use only dev deps that can be managed with cargo (see previous assumption)
    • install stuff locally (I personally don't like projects that force installing stuff globally and mess with my $PATH):
      I use the cargo install --root option to download a binaries into $PWD/.cargo/bin, this folder is added to the PATH by the Makefile, so all tasks must be run with make [task]
  • update the main workflow to use these tools
  • add a workflow to deploy a docs website: might be buggy atm, because we need to tweak the repo settings for it work and I am not admin
  • update README
  • git hooks set up (code formatting and conventional commit messages linting)

Test plan

I am looking for feedback on the dx.

  1. Clone the repo and run make setup: it should download some crates successfully and display the help at the end.

  2. Run make setup again: it should skip the download step and only show the help
    image

  3. Test all the tasks:

    • make help
    • make
    • make build
    • make build.docs
    • make check
    • make docs
    • make fix
    • make fmt
    • make lint
    • make test
  4. Test the conventional commit cli:

    make commit
    

    or

    git commit
    

    Should interactively let you writing a conventional commit message

  5. Test the commit-msg hook

    git commit --allow-empty -m "fix stuff"
    

    Should fail, telling you the commit message is not conventional

Formatting, linting, conventional commits lint pre-push hook.
GH workflows.
Docs deployment
@sripwoud sripwoud added the devops 🔧 Operations management and dev tools label Jul 1, 2024
@sripwoud sripwoud self-assigned this Jul 1, 2024
@sripwoud sripwoud requested review from 0xjei, curryrasul and cedoor and removed request for 0xjei and curryrasul July 1, 2024 14:29
@cedoor cedoor requested review from 0xjei and cedoor and removed request for cedoor and 0xjei July 2, 2024 10:00
Copy link
Member

@cedoor cedoor left a comment

Choose a reason for hiding this comment

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

When I run make setup I get the following error:

./.setup.sh: 2: set: Illegal option -o pipefail
make: *** [Makefile:41: setup] Error 2

.github/workflows/docs.yml Show resolved Hide resolved
.github/workflows/docs.yml Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
remove `-o pipefail` option
@sripwoud
Copy link
Member Author

sripwoud commented Jul 2, 2024

./.setup.sh: 2: set: Illegal option -o pipefail

@cedoor
Ups sorry
I want to use #!/bin/sh to have better portability compared to using a bash shebang #!/bin/bash, but -o pipefail isn't POSIX compliant. Hence your error.
We can remove this option anyway as nothing is piped in the script.

I didn't get this error locally because my /bin/sh is soft linked to bash.

ls -l /bin/sh
0777 lrwxrwxrwx - root : 16 Jan 19:10 /bin/sh -> bash

ec9ce9f should have fixed this

@cedoor
Copy link
Member

cedoor commented Jul 2, 2024

@sripwoud

I want to use #!/bin/sh to have better portability compared to using a bash shebang #!/bin/bash, but -o pipefail isn't POSIX compliant. Hence your error.

I see! It works now but I needed to install cmake. Should we add a note in the readme file with the list of packages to install to make it work properly?

@sripwoud
Copy link
Member Author

sripwoud commented Jul 2, 2024

@cedoor

It works now but I needed to install cmake

Ah damn. I had picked make precisely because I thought it was so universal that all OSes always have it pre installed.
Which OS/distro do you use?

Should we add a note in the readme file

Yes, I can also had a check and error message for this in the setup script.
I'd like to avoid having to deal with installing this kind of stuff globally cross platforms in the setup script, because this adds lot of complexity (different install steps for windows, mac, linux flavors etc...)

@cedoor
Copy link
Member

cedoor commented Jul 2, 2024

It works now but I needed to install cmake

Ah damn. I had picked make precisely because I thought it was so universal that all OSes always have it pre installed. Which OS/distro do you use?

I'm using Linux with Ubuntu Jammy!

@cedoor
Copy link
Member

cedoor commented Jul 2, 2024

@sripwoud make docs doesn't open my browser, it opens another OS app to manage my partitions 🤣 But this has something to do with Cargo doc I guess. The other commands are working fine!

@cedoor
Copy link
Member

cedoor commented Jul 2, 2024

@sripwoud

git commit --allow-empty -m "fix stuff" && git push

This fails, but the commit is created anyway.

Also, would it be possible to have a hook to create an alias for git commit -> make commit?

@sripwoud
Copy link
Member Author

sripwoud commented Jul 2, 2024

@cedoor

git commit --allow-empty -m "fix stuff" && git push

This fails, but the commit is created anyway.

This is the expected behavior. As the hook is a pre-push. The convco lint happens and fails only when trying to push.
Is there another way? the commit must be created so you can lint its message right?
EDIT: it seems that the post-commit exists, putting the commit linting stuff in a post-commit instead of pre-push hook would achieve what you want?

Also, would it be possible to have a hook to create an alias for git commit -> make commit?

do you mean you want that running git commit always calls convco? convco README suggests setting git core.editor to convco commit. I tried it but didn't like it: it'll will call convco on all git commands that deal with creating/editing commits, especially it will trigger on git rebase, which I don't like.

@cedoor
Copy link
Member

cedoor commented Jul 2, 2024

it seems that the post-commit exists, putting the commit linting stuff in a post-commit instead of pre-push hook would achieve what you want?

Yes, it would be in line with the other repos! It should be a pre-commit or commit-msg.

do you mean you want that running git commit always calls convco?

Exactly, this also is like the other repos work: https://github.com/privacy-scaling-explorations/zk-kit/blob/main/.husky/prepare-commit-msg

@sripwoud
Copy link
Member Author

sripwoud commented Jul 2, 2024

it seems that the post-commit exists, putting the commit linting stuff in a post-commit instead of pre-push hook would achieve what you want?

Yes, it would be in line with the other repos! It should be a pre-commit or commit-msg.

do you mean you want that running git commit always calls convco?

Exactly, this also is like the other repos work: https://github.com/privacy-scaling-explorations/zk-kit/blob/main/.husky/prepare-commit-msg

unfortunately the post-commit does not prevent the creation, it runs after the creation event (and before a pre-push), let me check what the other zk-kit repos do, maybe a NO_HOOK could do the trick here too

@sripwoud
Copy link
Member Author

sripwoud commented Jul 9, 2024

@cedoor could you try again the git hooks setup?
after
make setup

Now the linting should happen on the current commit msg, effectively preventing creating a wrong commit.

git commit

should also start the interactive CLI

@cedoor
Copy link
Member

cedoor commented Jul 9, 2024

Hey @sripwoud!

Got this error:

Writing hooks...
  .git/hooks/pre-commit (formatting)
  .git/hooks/commit-msg (conventional commits linting)
./.setup.sh: 85: EDITOR: parameter not set
make: *** [Makefile:41: setup] Error 2

cedoor
cedoor previously approved these changes Jul 9, 2024
@sripwoud sripwoud merged commit 770adbf into main Jul 9, 2024
4 checks passed
@sripwoud sripwoud deleted the chore/tooling branch July 9, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops 🔧 Operations management and dev tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define a basic toolset for this repository
2 participants