Skip to content
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 shell completions and README to build script #616

Conversation

jdsutherland
Copy link
Contributor

@jdsutherland jdsutherland commented Jul 13, 2018

Fixes #508. Possibly requires changes to facilitate #394? Additionally, might want to make relevant deletions from https://github.com/exercism/cli.exercism.io per exercism/DEPRECATED.cli-www#34?

Completion scripts currently live in the CLI website repository which is
deemed inappropriate

Update build script to add shell completions and README to tar/zips

Completion scripts currently live in the CLI website repository which is
deemed inappropriate

Update build script to add shell completions and README to tar/zips
@jdsutherland
Copy link
Contributor Author

jdsutherland commented Jul 13, 2018

From #394:

I need to figure out how to tell Homebrew where to put the auto-complete scripts, or if it has some good defaults.

I have no prior experience with the homebrew repo but it seems https://github.com/Homebrew/homebrew-core/blob/c53141dcda9d1a062eba89f77a5c02f09f6b5bd2/Formula/fzf.rb#L52 + fzf/shell might help.

Copy link
Member

@kytrinyx kytrinyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This will be such a relief to get done.

bin/build-all Outdated
@@ -13,6 +13,15 @@ ARMVAR=github.com/exercism/cli/cmd.BuildARM
# handle alternate binary name for pre-releases
BINNAME=${NAME:-exercism}

get_shell_completions() {
SHELLCOMP_BASEURL=http://cli.exercism.io/shell/exercism_completion
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the scripts in this repository instead of in the static HTML repo (as you suggested). That way this will be a simple file inclusion rather than a network call.

@jdsutherland jdsutherland force-pushed the migrate-completion-scripts-from-cli.exercism.io branch from 1cf2310 to f6880d9 Compare July 13, 2018 10:46
Copy link
Member

@kytrinyx kytrinyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the direction of this. A few things:

  • the build doesn't bail, but the resulting artifacts won't extract because it contains ../
  • let's move the BUILD.md into shell/ and rename it to README.md

@kytrinyx
Copy link
Member

Here's the error I got when I tried to extract the archive:

$ tar -xvzf exercism-mac-64bit.tgz 
x exercism
x ../shell/: Path contains '..'
x ../shell/exercism_completion.bash: Path contains '..'
x ../shell/exercism_completion.zsh: Path contains '..'
x ../BUILD.md: Path contains '..'
tar: Error exit delayed from previous errors.

Avoid including 'out' directory in generated archive
tar and zip differ in how to achieve this

tar -C will cd to dir before adding files.
zip cd subshell accomplishes the same thing.
@kytrinyx kytrinyx changed the base branch from nextercism to master July 13, 2018 17:15
@kytrinyx
Copy link
Member

FYI: I changed the base of this PR to point to master (I've been changing branches around, so what is on master now is what was previously on nextercism).

@kytrinyx kytrinyx merged commit a289457 into exercism:master Jul 14, 2018
@kytrinyx
Copy link
Member

❤️ 💛 💚 💙 💜 🖤 thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants