-
Notifications
You must be signed in to change notification settings - Fork 162
Add lilypond package support #546
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution!
Looks good to me on the principle. It adds ~22Mb to the image rootfs which is acceptable.
However, could you switch the package repository from testing to main (as indicated in the code comment) please?
Dockerfile
Outdated
&& apk add --no-cache -X http://dl-cdn.alpinelinux.org/alpine/edge/testing \ | ||
pdf2svg | ||
pdf2svg \ | ||
lilypond |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you install lilypond
from the main channel instead?
It is available in the same version as in the testing repository for Alpine 3.22: https://pkgs.alpinelinux.org/packages?name=lilypond&branch=v3.22&repo=&arch=x86_64&origin=&flagged=&maintainer=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated Dockerfile. Is it what you would like?
Moreover, Test wrote by Claude is enough, or do we need more thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated Dockerfile. Is it what you would like?
Yes, thanks! Using testing channel for packages on a production image should be avoided unless strictly required, because it does not provide guarantee of stability, neither presence of the packager in the future.
Moreover, Test wrote by Claude is enough, or do we need more thing?
I'm not an AI user at all so no advise on Claude code itself. It is a tool, you use it at your leisure.
However, as a reviewer of your contribution, I'm wondering the usual question when it comes about testing: "What do you want to test exactly?".
=> In your case, is testing the presence of the binary enough? Or is it a dynamic system which may fail when called if it misses a library? I suggest you look at the other examples and see if you can provide a real life example additionally to the "unit test" you've added?
Added lilypond to the Docker image to enable music notation rendering in AsciiDoc documents. This package is available from Alpine's edge/testing repository and allows users to generate musical scores within their documentation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
4306233
to
8cbd7e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Note: the provided tests are only checking for the binary, not for the full "end to end" testing with a real life example. But it is good enough.
Can be improved in a subsequent PR focused on non regression testing.
Added lilypond to the Docker image to enable music notation rendering in AsciiDoc documents. This package is available from Alpine's edge/testing repository and allows users to generate musical scores within their documentation.
🤖 Generated bats with Claude Code