-
Notifications
You must be signed in to change notification settings - Fork 795
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
Appimage automated build #1136
Appimage automated build #1136
Conversation
The node modules do not need to be deliverer with the appimage. Removing them from the AppDir saves 50MB.
Jenkins sets a $WORKSPACE environment variable. We'll use this if it's been set. Otherwise, use $HOME.
Codecov Report
@@ Coverage Diff @@
## develop #1136 +/- ##
===========================================
- Coverage 28.73% 28.40% -0.34%
===========================================
Files 410 410
Lines 12869 13104 +235
===========================================
+ Hits 3698 3722 +24
- Misses 9171 9382 +211
Continue to review full report at Codecov.
|
Do we have OS compatibility for app-image documented somewhere? |
Not yet. We have to decide on an official standard for how we test compatibility. It should work on "any modern Linux" as long as FUSE is available. There will, of course, be exceptions. |
log_message "downloading configuration" | ||
curl -L -s -o "$tmpfile" "$CONFIG_URL" | ||
is_valid_git_repo() { | ||
pushd "$1" 2>/dev/null || return 1 |
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.
Wait, what's the $1 argument?
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.
A local directory that potentially contains a valid git repo.
case "$1" in | ||
--agent-binary-dir) | ||
if [ -n "$2" ] && [ "${2:0:1}" != "-" ]; then | ||
agent_binary_dir=$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.
A lot of these flags are parsed with basically the same code, except the argument is assigned to a different variable, maybe there's an opportunity to remove duplication
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.
Bash functions are a little strange, so I can't get it quite as clean as I would like, but I've made a modest improvement.
appimage/build_appimage.sh
Outdated
missing_argument "$1" | ||
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.
why do we need the dash?
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.
Since we don't have any positional arguments, we don't. Good catch.
remove_node_modules() { | ||
# Node has served its purpose. We don't need to deliver the node modules with | ||
# the AppImage. | ||
rm -rf "$ISLAND_PATH"/cc/ui/node_modules |
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 will probably be slow. can we use npx remove-node-modules
?
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 ran a single test. The rm
ran in 1.7 seconds. the npx remode-node-modules
ran in 5.9 seconds.
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.
Looks good, but adress my questions before merging
What does this PR do?
Modifies the AppImage build script so that AppImage packages can be easily build locally by developers or by Jenkins.
Closes #1103
PR Checklist
Testing Checklist