zsh completion fixes #582

Closed
wants to merge 3 commits into
from

Projects

None yet

5 participants

@ghedo
Member
ghedo commented Dec 27, 2015

No description provided.

@mention-bot

By analyzing the blame information on this pull request, we identified @dfandrich, @bagder and @danielshahaf to be potential reviewers

@danielshahaf
Contributor

Haven't tested it, but looks good to me.

As to call_curl: usually I try to follow the rule that things that call execve() take a list of arguments and things that call system() take a single string argument. call_curl does not follow this rule. I'm not sure this matters — the script is unlikely to be extended in ways which would be affected by this implementation choice — but it's the only potential issue I see in the diff.

@bagder
Member
bagder commented Jan 10, 2016

I think it needs some extra precaution for cross-compile builds, as then there's no "just built" version of curl around to run.

@ghedo
Member
ghedo commented Jan 10, 2016

Various ways to fix cross-builds:

  1. Disable zsh generation by default
  2. Disable zsh generation when cross-building (don't know how to detect that)
  3. Make the call_curl failure a warning instead of an error (i.e. make the build proceed if calling curl fails). This is what's happening right without the second commit above, except that there's no output telling the user what's going on and it still generates a (broken) completion script.
@bagder
Member
bagder commented Jan 10, 2016

We set a symbol called CROSSCOMPILING for the automakefiles when cross-compiled, and you can see in the tests/Makefile.am for an example use of that. Basically you can do:

if CROSSCOMPILING
bla when cross-compiling
else
this is for native builds
endif

And the "right" piece will be put into the final makefile by configure.

@danielshahaf
Contributor

I think there are two parts to this fix. First part, since cross-builds currently generate a broken zsh completion script, fix that. The first part of this PR does that by having script/zsh.pl die(). Perhaps even forbid --with-zsh-functions-dir for cross-builds at configure (to fail as early as possible).

Second part, how might cross-builds generate a zsh completion script. The Perl script relies on the curl --help output. I see two alternatives: one, have the Perl script parse the help string from tool_help.c directly (after passing that file through the preprocessor to resolve the #ifdef's embedded in the definition of helptext (https://github.com/bagder/curl/blob/5d7c9379efa635a782d7380b1ef899dc1f7f5b29/src/tool_help.c#L46)); two, parse curl --help output not as part of the build process, but as part of the zsh code in the completion script — code that's run whenever the user types curl <TAB> for the first time in a shell session.

@ismail
Contributor
ismail commented Jan 11, 2016

Running curl --help each time one does curl is a very bad idea, just disabling zsh completion generation for cross builds makes more sense.

@danielshahaf
Contributor

It's not "every time one runs curl", it's only once per shell session: once you do curl <TAB> once in a shell session, the completer won't invoke curl again in that shell instance.

And please explain why you think it's a bad idea. Shelling out to the target command is common practice: 119 out of 801 completers in the zsh distribution invoke _call_program.

@ismail
Contributor
ismail commented Jan 11, 2016

If it's once per shell its more than fine, its good :-) Thanks!

ghedo added some commits Dec 27, 2015
@ghedo ghedo scripts: fix zsh completion generation
The script should use the just-built curl, not the system one. This fixes
zsh completion generation when no system curl is installed.
1b26af1
@ghedo ghedo zsh.pl: fail if no curl is found
Instead of generation a broken completion file.
a8e9168
@ghedo ghedo scripts: don't generate and install zsh completion when cross-compiling
9d60850
@ghedo
Member
ghedo commented Jan 11, 2016

@bagder added commit to disable completion generation/installation when cross-compiling (not sure if there's a smarter way to do it).

@bagder
Member
bagder commented Jan 11, 2016

Thanks a lot, these commits have now been merged as of commit ebfe00c. If anyone has any further improvements on the zsh completions, please just file another issue or PR!

@bagder bagder closed this Jan 11, 2016
@ghedo ghedo deleted the unknown repository branch Feb 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment