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

zsh completion fixes #582

Closed
wants to merge 3 commits into from
Closed

zsh completion fixes #582

wants to merge 3 commits into from

Conversation

ghedo
Copy link
Contributor

@ghedo ghedo commented Dec 27, 2015

No description provided.

@mention-bot
Copy link

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

@danielshahaf
Copy link
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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
Contributor

ismail commented Jan 11, 2016

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

Instead of generation a broken completion file.
The script should use the just-built curl, not the system one. This fixes
zsh completion generation when no system curl is installed.
@ghedo
Copy link
Contributor Author

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
Copy link
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 zsh branch February 17, 2016 20:33
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants