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

feat: pre-commit hook with no prerequisites #371

Merged
merged 3 commits into from
Jan 15, 2023

Conversation

l0b0
Copy link
Contributor

@l0b0 l0b0 commented Jan 15, 2023

@kamadorueda Are you OK with the new no-prerequisites hook replacing the hook which requires Nix? Otherwise I could just rename it.

Closes #370.

@kamadorueda kamadorueda merged commit 1d27a95 into kamadorueda:main Jan 15, 2023
@l0b0 l0b0 deleted the feat/install-alejandra-using-cargo branch January 15, 2023 20:46
@l0b0
Copy link
Contributor Author

l0b0 commented Jan 15, 2023

That was quick! Does it work for you though? I'm getting an error message:

error: found a virtual manifest at /home/username/.cache/pre-commit/repos0p3hajw/Cargo.toml instead of a package manifest

Is it possible that the alejandra Cargo.toml doesn't have enough information for pre-commit to install it?

@kamadorueda
Copy link
Owner

I trust your judgment, since you use pre-commit you have a better idea about what works best

I think the error you get is because Alejandra uses cargo workspaces

  • looks like https://pre-commit.com/#rust runs cargo install --bins, which doesn't work
  • cargo install --bins --path src/alejandra_cli works, not sure if there is a way to pass those arguments to cargo

@l0b0
Copy link
Contributor Author

l0b0 commented Jan 15, 2023

Doesn't look like we can pass arguments to Cargo (I imagine it would be a huge security issue target). Some options:

  • Make the root Cargo.toml build the CLI executable, to satisfy the pre-commit standard requirements. 👍
  • Create a separate repo with only the pre-commit stuff (some repos do this, although it must be a pain to maintain). 👎
  • Revert this PR. 😢

What do you think?

@kamadorueda
Copy link
Owner

Make the root Cargo.toml build the CLI executable

This one may be possible, but there is some setup effort to it (moving files around, updating CI, and so on). Right now we have 2 crates (Alejandra, alejandra_cli) so we can use the Alejandra crate both in the CLI and in the Demo Website, as long as we maintain this property, I think I would be fine with reorganizing the project layout

But before that:

  • Looks like there is an args argument that we can pass to the hook, would this allow passing the flags we need? https://pre-commit.com/#hooks-args
  • Would publishing the alejandra_cli to crates.io help with something? this way cargo-install does not need the source code, and just reference a name and version
  • Would defining a bash script that does the cargo install needed, be easier to maintain?

Just trying to see if there is a simpler approach than moving the whole repo around

@l0b0
Copy link
Contributor Author

l0b0 commented Jan 15, 2023

I don't think so. Those are the arguments sent to the hook, not to the build process.

  • Would publishing the alejandra_cli to crates.io help with something? this way cargo-install does not need the source code, and just reference a name and version

I'm not sure; this is too much into the details of Rust/pre-commit for my knowledge. If cargo install --bins --root . is capable of downloading from crates.io, then I guess it would work.

  • Would defining a bash script that does the cargo install needed, be easier to maintain?

I don't know. Looking through the options:

  • language: script doesn't seem to have any separate settings for installing, which means it'd try to install every time it's run.
  • Docker would work with a simple Dockerfile.
  • Docker image would be faster if there was a published Docker image.

@kamadorueda
Copy link
Owner

Cooking a container image and publishing it to a registry should be very easy, would this be a solution to what you try to accomplish? what would be the advantages of this vs using a pre-compiled binary from the releases page? https://github.com/kamadorueda/alejandra/releases , both would be more or less "downloading a binary and running it"

I mention the language:script because same as the language:rust runs cargo under the hood, we can just make the script do that, store a cache somewhere, and so on so that it doesn't compile things every time

@l0b0
Copy link
Contributor Author

l0b0 commented Jan 15, 2023

Pre-compiled would probably be best, since it wouldn't require Docker and would be faster because of no building.

@kamadorueda
Copy link
Owner

feel free to go with the one that works best for you. If it works well for you, it'll probably be most useful for other users, too

@l0b0
Copy link
Contributor Author

l0b0 commented Jan 15, 2023

In the meantime, feel free to revert this if it's going to block any releases. Also, thank you for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pre-commit hook without prerequisites
2 participants