-
Notifications
You must be signed in to change notification settings - Fork 518
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
CLI Compatibility for Linux #231
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 starting this @jfgordon2!
I wanted to sit down and test this myself to confirm the CLI launches as expected, but otherwise this is looking good!
@@ -4,8 +4,10 @@ set -e | |||
|
|||
PROFILE_D_FILE="/etc/profile.d/github-desktop.sh" | |||
INSTALL_DIR="/opt/${productFilename}" | |||
CLI_DIR="$INSTALL_DIR/resources/app/static" |
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.
Because we have full control over how the app is built for packaging, I think we can make the github
shortcut live wherever we like. Other OSes have their own constraints:
- Windows includes the versions in the path, so we need to update the shortcut after launch
- macOS may requires elevation to install it at a predictable location, so we have an in-app flow to check and prompt
I'm going to leave this here for now because it's working, but if I have any better ideas I'll let you know.
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.
AH that would certainly make it easier; then we just can create a symbolic link in the /usr/bin directory
ln -s $INSTALL_DIR/resources/app/static/github /usr/bin/github
Added github.sh script to launch electron app from CLI, and added linux environment to CLI file; tested successfully in Elementary OS 5.1.2; still needs modification on linux-after-install script for PATH
Removed restriction on electron CLI for linux, added shim to find binary, and added to path; also bugfixed check for editors to also include linux enums to allow linux-only editors to not error out, and added Elementary Code and Elementary Terminal
Co-Authored-By: Brendan Forster <brendan@github.com>
Co-Authored-By: Brendan Forster <brendan@github.com>
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 is feeling really close @jfgordon2 - I've been able to test it out and I can confirm it works on my end on Ubuntu 19.10. Once we've addressed the last couple of things I'll pull this into an update after I've pushed 2.4.0 out to everyone.
@@ -0,0 +1,28 @@ | |||
#!/bin/sh |
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.
Currently this file does not have the executable bit set after install:
$ ls -l "/opt/GitHub Desktop/resources/app/static"
total 960
...
-rw-r--r-- 1 root root 760 Mar 20 11:14 github
...
Should we just chmod a+x
this file so everyone can launch 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.
ah, good catch! I might have done that without thinking or assumed it was done on the folder level on installation; yes, we'd want to make it executable
@jfgordon2 don't worry about the warning here about conflicts - once you've applied that fix to the file mode for the script I'm happy to figure out how to rewrite the history so the diff is easier to follow. |
@shiftkey I probably should have just rebased this to remove the confusing commits; I've updated this to add file permissions and removed your |
Added binary to /usr/bin to avoid having to add to PATH
@jfgordon2 I've rebased to drop those unnecessary commits, should be good to land once this goes green. Then I'll cherry-pick it over to the release branch so it can go out with #237. |
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 working with me to get this supported @jfgordon2.
Can't wait to get this out for others to enjoy!
Closes #32
Description
Tested in Elementary OS 5.1.2
Screenshots
Release notes
Notes: