-
Notifications
You must be signed in to change notification settings - Fork 345
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
Add AppImage build script #739
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.
Thank you for your contribution, it's very useful!
I will add that to the standard build process as soon as it's finalized.
Feel free to answer my comments.
elif [[ ! -d "$1" ]]; then | ||
echo "Error: $1 is not a directory" | ||
show_usage_and_exit | ||
fi |
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 add a check that verifies that the script is being executed on the right OS (I guess this is linux-only?) and architecture (x86_64 - we may support more in the future)?
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'd rather require an $ARCH
env var to be set instead, which can be evaluated in the last lines (where appimagetool is called). Detecting the architecture is horribly hard. In a CI script, one can easily add that environment variable manually, though. It's not subject to future changes.
The script can be run on any unix that can execute appimagetool (this includes FreeBSD with the linuxulator, I think). Limiting this to Linux does not make any sense to me. Could you please state how such a check would help you?
export VERSION | ||
|
||
# create AppRun | ||
[[ ! -f AppRun ]] && ln -s Natron AppRun |
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.
As you suggested, the script should also expose NatronRenderer, how easy is it?
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.
Depends on how you want it to work. I think some graphics utility evaluated a --executable
parameter, others use environment variables. Do you have a UX in mind?
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 never used appimage. Can NatronRenderer be in the user PATH?
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.
Anything is possible. For instance, you could have a wrapper that calls the AppImage.
Think of an AppImage as a filesystem image with a runtime that FUSE mounts that image and executes a defined entrypoint.
|
||
show_usage_and_exit() { | ||
echo "Usage: $0 <dir>" | ||
exit 2 |
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.
Please say in the help that
should be the result of... extracting the no-installer linux binary distribution?Actually, I would like it better if the script would even take care of extracting that distribution from a downloaded file into a temp dir, and cleanup the mess at the end
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.
This comment seems broken, please revise.
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.
Please say in the help that <dir>
should be the result of... extracting the no-installer linux binary distribution(?).
Actually, the script could even take care of extracting that distribution from a downloaded file into a temp dir, and cleanup the mess at the end.
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.
It could, but that doesn't make sense to me, really. It might make sense in your build infrastructure, though. I do not know how you plan to integrate this.
If you need such a script, you could add it as a second script which implements the downloading, extraction etc., and then calls this script. This script follows the Unix spirit, where there should be one tool for one job, and it should do that job well.
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.
Yes you're right, we should run this script at the same time we build the tarball.
What I don't understand is that there seem to be a usr/ hierarchy in the tarball, but you leave most stuff out of it.
Shouldn't everything be moved inside usr/, rather than live in bin/ docs/ etc? What's the rule for AppImage?
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.
Sorry for not replying in time. Feel free to move everything to a usr/
subdirectory. It's common for AppImages, maybe even "best practice", but not a requirement technically. I'll send a follow-up PR.
Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:
PR Description
What type of PR is this? (Check one of the boxes below)
What does this pull request do?
See #318.
Adds an AppImage build script that converts an extracted Natron tarball into a functional AppImage.
NatronRenderer has not been exposed yet, but this is a trivial change that can be added at any time.
Have you tested your changes (if applicable)? If so, how?
The built AppImage works very well.