crystal-ameba / ameba

A static code analysis tool for Crystal
https://crystal-ameba.github.io
MIT License
525 stars 39 forks source link

Restructure Ameba Distribution #470

Open devnote-dev opened 2 months ago

devnote-dev commented 2 months ago

I recently read an issue involving Ameba; a contributor stated they didn't believe it should be installed and compiled as a development dependency, but rather distributed and installed like any other external tool. I (also recently, coincidentally) encountered an odd issue when running Ameba in one of my projects: I have several folders that link to PATH so running ameba can call any of them which can be any version and return varying results. And that's exactly what happened 😄

Now, to remove these binaries wouldn't necessarily solve the issue as different projects use different versions of Ameba, some of which require refactoring to support newer versions. I also can't have multiple versions of Ameba available in PATH because of the aforementioned issue. But what if I could have one version of Ameba with multiple versions?

I propose that all rules be bound to a version requirement and introduce a --version flag (or command, whatever works best) that accepts a valid Ameba version. Running ameba --version 1.5.0 would only enable rules matching the < 1.5.0 requirement. Running ameba would default to the latest available version, that is, the version of that binary. This process is similar to Rust editions (if I've understood that correctly) and would remove the need to depend on multiple local versions.

Alongside this, I propose that Ameba is released in official distribution channels and discourage the current practice of local development installations with Shards (perhaps a compile time warning, similar to how Crystal's i_know_what_im_doing flag works).

straight-shoota commented 2 months ago

I completely agree. But the ameba version should generally be recorded in .ameba.yml. There's already a discussion about keeping the ruleset consistent in #448.

You're totally right that in order to remove ameba from an implicit postinstall, we need to improve and expand its distribution channels. It needs to be easy to get a recent version installed.

There are currently couple of packages available in the distribution package repos of Alpine Linux, AUR and nixpkgs (https://repology.org/project/ameba/versions).

devnote-dev commented 2 months ago

But the ameba version should generally be recorded in .ameba.yml.

I hadn't even thought about that but it's perfect! 👍

As for distribution, I believe that providing compiled binaries as part of each release would greatly support this. I'm not sure how practical it would be to provide a couple previous versions' releases, but it's definitely viable going forward.

straight-shoota commented 2 months ago

I'm not sure how practical it would be to provide a couple previous versions' releases, but it's definitely viable going forward.

If binaries are attached to GitHub releases for example, earlier versions would automatically be available. However I don't think there should be much need for installing older versions if newer ones are able to emulate the configuration of previous versions.

devnote-dev commented 1 month ago

I'll be starting work on this shortly, is there a preferred implementation for this? I've been familiarising myself with the codebase and there seems to be a lot of heavy macro processing. (cc: @Sija)

Sija commented 1 month ago

@devnote-dev I've created a PR which implements rule versioning as the first step towards that. Feel free to comment on that and propose further action points.

devnote-dev commented 1 month ago

This may be unrelated but when building that branch and running Ameba, I got this error:

image

Sija commented 1 month ago

@devnote-dev yup, looks like unrelated Crystal Windows-related bug, or a variant of #473.

nobodywasishere commented 6 days ago

The downside to this (even though I'm for it due to not wanting to have a separate version of ameba in each project), is that there's no way to create custom per-project plugins like there are with the current system.

https://crystal-ameba.github.io/2019/07/22/how-to-write-extension/

straight-shoota commented 6 days ago

that there's no way to create custom per-project plugins like there are with the current system.

You can still have custom extensions in your codebase or a dependency and use it to build a custom version of ameba. I don't think this would really change much at all since you already have to explicitly build ameba in this case anyway.

devnote-dev commented 3 days ago

With rule versioning now implemented, we can start looking at distribution (thank you Sija ❤️).

For Windows this should be pretty straightforward as Scoop is a generally widely accessible package manager. We could also support WinGet but that requires a separate packaging process (Scoop just reads from a package file) and I haven't really seen anyone using it for Crystal related work.

For Linux, adding the common tools (APT, Pacman, etc.) should be fine. I'm not aware of other managers that need to be supported but I can do some research into that.

Homebrew should be fine as is, the only thing we'd need to change is the compile time flag to disable the warning — any strong naming towards this or should we go with the one in OP?

straight-shoota commented 3 days ago

I'd recommend as the very first step to provide generic builds on the GitHub release. That would provide a good base. And I imagine it could already satisfy quite many use cases.

On many platforms installing ameba could be as simple as downloading a statically linked build and making it executable.

curl -L -o ~/bin/ameba https://github.com/crystal-ameba/ameba/releases/download/1.7.0/ameba-1.7.0-linux-x86_64 && chmod +x ~/bin/ameba

We might not even need to bother about providing packages. That's quite a rabbit hole. Most use cases should be perfectly fine with a simple mechanism to install a statically linked executable. So it should be fine to leave packaging to downstream package maintainers.

devnote-dev commented 2 days ago

FTR Scoop and Linux would all depend on a GitHub release being available, so simply downloading a static build and making it executable would be possible in all cases.

On the package manager side of things, there does need to be a configuration. Scoop is literally just a JSON file, whereas APT is a bit more work (and I have no idea about Pacman/AUR).