registry: add imagemagick (aqua:ImageMagick/ImageMagick)#10118
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the ImageMagick registry configuration by replacing the existing conda and asdf backends with aqua, and enabling the test command. The reviewer recommends retaining the original conda and asdf backends alongside the new aqua backend to prevent regressions for existing users.
thernstig
changed the title
Greptile SummaryThis PR adds
Confidence Score: 5/5This is a small, targeted registry change with a single modified file; the Windows-only platform scoping is well-justified and the test template handles all relevant version formats. The change adds one new backend with an explicit platform guard and a corrected test. The git history shows the Windows-only restriction is a deliberate choice (Linux AppImage/FUSE support deferred). No logic errors, no broken contracts, and no security surface introduced. No files require special attention. Important Files Changed
Reviews (13): Last reviewed commit: "fix: normalize ImageMagick test version ..." | Re-trigger Greptile |
|
I am not able to run edit: wrote an issue here about this #10121 Regarding the other failure imagemagick has not been released yet: https://github.com/aquaproj/aqua-registry/releases |
|
It was released here: https://github.com/aquaproj/aqua-registry/releases/tag/v4.518.0. Not sure why it says it is not in the registry. |
|
@risu729 help me here. |
thernstig
force-pushed
the
feat/imagemagick
branch
from
15c5add to
e365210
Compare
|
@risu729 updating it manually did not make this repo happy. I thought that was what you indicated I should do. |
|
@thernstig You need to wait until the next mise release. |
|
I am checking this, as I thought they meant to support everything but Windows on Arm. Or will mise not even allow this in even in that case, if everything is supported but WIndows on Arm 64? |
|
That's the opposite. Check aqua docs |
|
@risu729 I saw that, but I thought their intention was to support everything but windows/amd64, but it seems only windows/amd64 is supported. I have asked here if the rest of the support will be added, so we can keep this on hold, see aquaproj/aqua-registry#50281. They now added support in https://github.com/aquaproj/aqua-registry/pull/54605/changes. My question is: Is this support now enough to make it into mise? Or what is required for it to be allowed to be added here? Is the support now enough? |
|
mise registry supports os/arch pairs. I think you can use it |
thernstig
force-pushed
the
feat/imagemagick
branch
from
ffd8150 to
5f115d3
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an aqua backend and activates the ImageMagick ChangesImageMagick Registry Configuration
Test runtime and packaging updates for FUSE
🎯 4 (Complex) | ⏱️ ~40 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
I updated it to only allow for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
registry/imagemagick.toml (1)
7-7: Test command looks correct for ImageMagick 7+.The test configuration using
magick -versionwith expected output patternImageMagick {{version}}follows the correct schema and will be properly validated by the test tool. Themagickcommand is the unified interface introduced in ImageMagick 7, which is appropriate for modern installations.To confirm the output format matches expectations, you can optionally verify with:
#!/bin/bash # Description: Verify ImageMagick version command output format # Try to find and run ImageMagick if available in the environment if command -v magick &> /dev/null; then echo "=== Testing magick -version output ===" magick -version | head -5 elif command -v convert &> /dev/null; then echo "=== ImageMagick 6 detected (convert command) ===" convert -version | head -5 else echo "ImageMagick not found in current environment (expected in test environment)" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@registry/imagemagick.toml` at line 7, The test entry currently uses the `test` table with cmd "magick -version" and expected "ImageMagick {{version}}"; keep this for ImageMagick 7+, but add a fallback check for ImageMagick 6 by supporting `convert -version` or conditionally running `convert` when `magick` is absent; update the test definition (the `test` key and its cmd/expected values) to either include both commands as alternatives or implement a small wrapper script that tries `magick -version` first and falls back to `convert -version`, ensuring the expected pattern remains correct for each variant ("ImageMagick {{version}}" for `magick` and matching `convert` output for v6).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@registry/imagemagick.toml`:
- Line 7: The test entry currently uses the `test` table with cmd "magick
-version" and expected "ImageMagick {{version}}"; keep this for ImageMagick 7+,
but add a fallback check for ImageMagick 6 by supporting `convert -version` or
conditionally running `convert` when `magick` is absent; update the test
definition (the `test` key and its cmd/expected values) to either include both
commands as alternatives or implement a small wrapper script that tries `magick
-version` first and falls back to `convert -version`, ensuring the expected
pattern remains correct for each variant ("ImageMagick {{version}}" for `magick`
and matching `convert` output for v6).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 09de9916-601d-4cac-b840-dc90ce6fd1f3
📒 Files selected for processing (1)
registry/imagemagick.toml
|
@jdx I am unsure what to do here, as AppImage requires fuse, see:
It should be possible to get around this to run the AppImage with Is this possible here? |
|
I don't know either |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/registry.yml:
- Around line 240-242: The registry workflow's change filters need to include
the new FUSE/AppImage runtime files so registry CI runs when those are modified:
update the path filters in the registry.yml check-changes/workflow-check section
to include e2e/run_test and packaging/e2e/Dockerfile (and any new FUSE-related
dirs referenced by the docker run change), and mirror the same additions at the
other filter block mentioned (lines around the second occurrence). Locate the
path-filter entries in registry.yml (the check-changes/workflow-check filter
blocks) and add those exact paths so edits to the FUSE/Dockerfile or e2e runner
no longer bypass registry CI.
In `@packaging/e2e/Dockerfile`:
- Line 47: The Dockerfile currently installs only libfuse2t64 but needs the
fuse3 package so /usr/bin/fusermount3 actually exists; update the apt install
line to include (or replace libfuse2t64 with) fuse3 and ensure the symlink block
that creates /usr/bin/fusermount from /usr/bin/fusermount3 is meaningful by
running it after fuse3 is installed. Specifically, change the package list
referencing libfuse2t64 to include fuse3 (or add fuse3), then keep the existing
symlink logic that references /usr/bin/fusermount3 -> /usr/bin/fusermount so the
helper binary is present when the symlink is created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c0145a0-08eb-4f75-9905-cbe7d8d14d7a
📒 Files selected for processing (3)
.github/workflows/registry.ymle2e/run_testpackaging/e2e/Dockerfile
|
@jdx this cannot be solved without your help. We need to update the Docker image to include fuse, which I tried to do, as done here https://github.com/AppImage/AppImageKit/wiki/FUSE But I cannot build and push the new image myself. |
|
If the tool would need FUSE as a prereq I don't think it's an improvement over conda to begin with |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 3 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 4 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
why are we getting rid of conda here? I don't understand the value |
|
@jdx I am unsure what you mean with "getting rid of"? Is it not supported still via https://github.com/jdx/mise/pull/10118/changes#diff-f322a53bf146c72239948186ecada0e05f5189a503c589da27ba156c22e41856R1-R8? If you wonder why we wanted to add aqua, I thought it was preferred? I read all docs and that is the general gist I got out of it, especially reading https://mise.jdx.dev/registry.html where e.g. conda is tier 2. It depends on if we can support AppImage in this case or not. Seems you do not want this? I can mention I personally had trouble with conda, even though I did not write a bug report. It worked eventually, but it took forever to install and timed out. Worked on retries. My only goal is/was to help future people of imagemagick installs 😃 |
I'll try to add the context here:
Imo the options are:
WDYT? |
|
I would just use it for windows. If there isn't a reason for moving away from Conda I don't understand why we would |
|
@jdx it is much faster, and less error-prone. I know this for a fact since I installed imagemagick via conda 3-4 weeks ago and it was a) very slow compared to other aqua tools b) failing at first at timeouts, twice, but later workd. Did not investigate why. Seems you do not wish to support AppImage type2? |
|
I think there is a lot of risk with adding AppImage support that I don't want to take on without a good reason |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 5 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
@thernstig maybe just add aqua:ImageMagick as Windows-only for now, and open issue on ImageMagick repo to build linux archive as 7z as well (in addition to AppImage), so we can add it to mise registry for faster install as well? |
Will do this. Do you mind opening that issue since you wrote and fixed the original issue on Aqua? |
|
Don't think the latest errors are caused by my change. |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Install libfuse2t64 in the e2e Docker image and enable /dev/fuse access with SYS_ADMIN for test-tool container runs.
Revert the Linux/AppImage follow-up work that added FUSE requirements to the e2e Docker path and registry workflow checks. Keep the ImageMagick aqua mapping scoped to Windows while broader platform support is validated separately.
thernstig
force-pushed
the
feat/imagemagick
branch
from
acec9e4 to
68f5cba
Compare
|
@suchipi reports long build times on Linux too: #8476 (comment)
Let's hope ImageMagick will add the static Linux build archive: ImageMagick/ImageMagick#8792 (comment there if you need it) |
4 participants
Summary by CodeRabbit
Chores
Tests