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

Problem with pkg-config #982

Closed
nlw0 opened this issue Aug 29, 2013 · 22 comments
Labels

Comments

@nlw0
Copy link

@nlw0 nlw0 commented Aug 29, 2013

I am trying to compile an opencv program, using pkg-config to help assemble the compile command. In bash I would use

g++ -o code code.cc `pkg-config opencv --cflags --libs`

and the corresponding in fish would be

g++ -o code code.cc (pkg-config opencv --cflags --libs)

But something happens, and it seems the correct command is not being executed. If I echo that, and then copy-paste the command, it works. What can be happening?

@xfix

This comment has been minimized.

Copy link
Member

@xfix xfix commented Aug 29, 2013

pkg-config separates arguments by spaces. This "works" in bash, but the behavior is considered buggy (at least for me), as having "sane" behavior in bash (separating by newlines, not by spaces - useful when you have list of files, for example), is difficult in bash.

( IFS=$'\n'; perl -E'say join " ", map "<$_>", @ARGV' $(exec echo a b\ c $'d\ne') )

Fish tries to make common behavior obvious, and doesn't automatically separate by spaces (making processing lists separated by newlines more convenient). Sadly, pkg-config separates its list by spaces, and bash behavior is more useful here (rare case, I have to say). You may want to use eval, or use something to replace spaces with newlines.

eval g++ -o code code.cc (pkg-config opencv --cflags --libs)

Or replace spaces with newlines

g++ -o code code.cc (pkg-config opencv --cflags --libs | perl -pe 's/\s+/\n/g')
@nlw0

This comment has been minimized.

Copy link
Author

@nlw0 nlw0 commented Aug 29, 2013

OK, I that makes sense... But I still don't understand what is happening exactly. Is the output from (pkg-config ...) being treated as a single argument, like it were in quotes? And it had to have newlines in order for fish to parse it into separate arguments to assemble the command? Because I may not be the only one who thinks it is not really very intuitive that newlines are the way to go since when we type the command on the prompt we are using spaces.

@nlw0

This comment has been minimized.

Copy link
Author

@nlw0 nlw0 commented Aug 29, 2013

Just leaving that here. Instead of always calling pkg-config with a pipe to use that workaround, you can create a function to replace pkg-config...

function my-pkg-config
pkg-config $argv | perl -pe 's/\s+/\n/g'
end

@ridiculousfish

This comment has been minimized.

Copy link
Member

@ridiculousfish ridiculousfish commented Aug 30, 2013

This is a good use case for the proposed list builtin #445

@zanchey

This comment has been minimized.

Copy link
Member

@zanchey zanchey commented Nov 28, 2015

Seeing as it comes up fairly frequently - #1947, #2568, #2569 - should fish provide a wrapper around pkg-config that just DTRT?

@faho

This comment has been minimized.

Copy link
Member

@faho faho commented Nov 28, 2015

As in

function pkg-config
    command pkg-config $argv | string split " "
end

?

I'd be okay with that, though the comments should make it clear what's going on.

@faho

This comment has been minimized.

Copy link
Member

@faho faho commented Dec 3, 2015

That actually breaks for e.g. pkg-config --list-all.

@Streppel

This comment has been minimized.

Copy link

@Streppel Streppel commented Feb 14, 2016

Just ended here, after struggling for about two hours (mea culpa, indeed). my face when I saw @xfix explanation. A wrapper would be nice - I'll be following @nlw0 suggestion. Thanks.

@tonytonyjan

This comment has been minimized.

Copy link

@tonytonyjan tonytonyjan commented May 24, 2016

below is my solution using eval:

eval g++ -o code code.cc (pkg-config opencv --cflags --libs)
@zanchey

This comment has been minimized.

Copy link
Member

@zanchey zanchey commented Jul 6, 2016

Perhaps the pkg-config developers could alter the output for fish. I've asked them at https://bugs.freedesktop.org/show_bug.cgi?id=96830

If not, a wrapper function that does if status --is-command-substitution before transforming the next might be the best method.

@krader1961 krader1961 modified the milestone: fish-tank Nov 17, 2016
@jbarlow83

This comment has been minimized.

Copy link

@jbarlow83 jbarlow83 commented Dec 18, 2016

Just ran into this with python3-config -- worth noting that there are some forms of {package}-config that wrap or behave similarly to pkg-config that exhibit this problem.

I think the best course of action is to wrap pkg-config and friends - then it can be fixed in fish for all existing versions of pkg-config rather than waiting for the (somewhat reluctant sounding) developer of pkg-config to maybe include a patch.

@faho

This comment has been minimized.

Copy link
Member

@faho faho commented Dec 18, 2016

Unfortunately, the wrapper solution doesn't seem workable.

The problem is that pkg-config has a lot of options, and only some of those are space-separated - e.g. "--list-all" prints two padded columns. So we'd be stuck parsing pkg-config's arguments for it.

This needs to be documented in general (not just for pkg-config), and it'd be swell if pkg-config would get a patch - we could even add a wrapper function that calls "pkg-config --fish-syntax".

I mean I can't currently see in what way pkg-config would break if it would just always use newlines (AFAIK every shell also splits on newlines), but I can also see why the pkg-config maintainers would be reluctant to change that for what is a small portion of their users, with little to no benefit for "everyone else".

@faho faho added the docs label Dec 18, 2016
@zanchey

This comment has been minimized.

Copy link
Member

@zanchey zanchey commented Dec 18, 2016

The pkg-config maintainer says:

I suppose I'd take a patch that prints each argument on a newline if it's not too intrusive. I'd prefer not to autodetect it but rather have it be an explicit option (--fish-syntax?). Not really sure, this is an unusual environment. I don't really see how command substitution is going to work well in fish unless all other tools are changed to output newlines or fish handles this natively.

@krader1961 krader1961 added this to the fish-future milestone Mar 15, 2017
@faho

This comment has been minimized.

Copy link
Member

@faho faho commented Dec 13, 2017

I think I completely missed @zanchey's idea of only doing it in command substitutions.

That actually sounds like it could work - it might make people think we split on newlines, but at this point, I've heard about this issue enough that I'm inclined to believe it's bad enough to be worth it.

However, while pkg-config is certainly the most well-known of these things, there are a whole bunch of other tools like it. On my system I count:

aalib-config
caca-config
croco-0.6-config
cups-config
curl-config
freetype-config
gdlib-config
gpg-error-config
gpgme-config
gphoto2-config
gphoto2-port-config
ijs-config
imlib2-config
js24-config
krb5-config
ksba-config
libassuan-config
libgcrypt-config
libmikmod-config
libotf-config
libpng-config
libpng-config-32
libpng16-config
libpng16-config-32
m17n-config
Magick++-config
MagickCore-config
MagickWand-config
ncursesw6-config
npth-config
nspr-config
onig-config
pcap-config
pcre-config
python-config
python2-config
python2.7-config
python3-config
python3.6-config
python3.6m-config
rasqal-config
redland-config
sane-config
sdl-config
sdl-config-32
sdl2-config
smpeg-config
taglib-config
taglib-extras-config
xml2-config
xmlsec1-config
xslt-config

(generated with the quite dangerous for program in /usr/bin/*config*; eval $program --libs ^/dev/null | grep "^-[lL]" >/dev/null; and echo $program; end)

That's 52 things that all need a function along the lines of

function $tool-config
    if status is-command-substitution
        command $tool-config $argv | string split ' '
    else
        command $tool-config $argv
    end
end

For comparison, we currently have a total of 185 function files. This would inflate that number quite a bit.

However, the only alternative I can come up with is to do something like

for tool in #list
    function $tool

in share/config.fish. Which strikes me as quite ugly.

Any ideas?

@mqudsi

This comment has been minimized.

Copy link
Contributor

@mqudsi mqudsi commented Mar 5, 2018

I'm highly against writing a function for each of those commands both because it's unmaintainable and because it's a workaround against something that should be somehow possible in fish (i.e. command substitution without escaping).

@faho

This comment has been minimized.

Copy link
Member

@faho faho commented Mar 6, 2018

it's a workaround against something that should be somehow possible in fish (i.e. command substitution without escaping).

?

I don't think you quite understand the issue. What is needed here is to split on spaces, not just newlines. And that is possible - just use string split " ". The problem is that users need to know about that, and use it with these tools pretty much always.

@mqudsi

This comment has been minimized.

Copy link
Contributor

@mqudsi mqudsi commented Mar 6, 2018

The problem is that splitting on spaces is just a workaround that does not address the core issue and also does not cover anything other than the most basic case.

mqudsi@ZBook ~/r/fish-shell> printf "Hello %s, What can I do for you %s?" "Mahmoud Al-Qudsi" "today"
Hello Mahmoud Al-Qudsi, What can I do for you today?⏎                                                                                       
mqudsi@ZBook ~/r/fish-shell> printf "Hello %s, What can I do for you %s?" (echo "Mahmoud Al-Qudsi" "today")
Hello Mahmoud Al-Qudsi today, What can I do for you ?⏎                                                                                      
mqudsi@ZBook ~/r/fish-shell> printf "Hello %s, What can I do for you %s?" (echo "Mahmoud Al-Qudsi" "today" | string split " ")
Hello Mahmoud, What can I do for you Al-Qudsi?Hello today, What can I do for you ?⏎                                                         
mqudsi@ZBook ~/r/fish-shell> printf "Hello %s, What can I do for you %s?" (echo \"Mahmoud Al-Qudsi\" \"today\" | string split " ")
Hello "Mahmoud, What can I do for you Al-Qudsi"?Hello "today", What can I do for you ?⏎
@faho

This comment has been minimized.

Copy link
Member

@faho faho commented Mar 6, 2018

The problem is that splitting on spaces is just a workaround that does not address the core issue and also does not cover anything other than the most basic case.

Aaaah, now I get what you want, and now I get what you mean in #4781 with "unescaped" substitution.

So, I gather you assume command substitutions work like this:

  • Fish executes the command, buffers the output

  • It then goes through that output and escapes (by inserting a \\ or similar) all the things that would end a token, except for newlines

  • It then runs all that output through the tokenizer, which causes one token per line to appear

If so, that's not what happens. Fish doesn't escape only to then undo that escaping.

What happens is:

  • Fish runs command, buffers output.

  • Fish goes through the output and splits it on newlines (unless $IFS is not set, in which case it doesn't split at all).

  • Fish inserts the resulting tokens into the arguments to be passed to the next command.

Adding some way to read tokens (as if they appeared on the commandline) is something we definitely want to add. See #3823.

However, that's not what is needed here - bash doesn't tokenize this either (it splits on every char in $IFS, which defaults to space, tab and newline), so the command wouldn't work with bash either if it expected tokenization. POSIX's (and therefore bash's) read builtin works like that by default, and it's a frequent source of problems. Which is why http://mywiki.wooledge.org/BashFAQ/001 (yes, question number 1) recommends using IFS= read -r line and says:

You should almost always use the -r option with read.

Anyway, let's track tokenization in #3823. This is about adding a workaround to a common issue with the *-config tools, that just requires splitting on spaces.

@mqudsi

This comment has been minimized.

Copy link
Contributor

@mqudsi mqudsi commented Mar 6, 2018

Thanks. Yes, now you get what I meant :)

@hugomg

This comment has been minimized.

Copy link

@hugomg hugomg commented Apr 28, 2018

Looking at the previous discussion on this topic, I wonder if the best idea presented so far would be to have fish provide a builtin function that can be used in a pipeline to convert a POSIX-escaped list of arguments (as produced by pkg-config and other tools) into a fish-escaped list of arguments (that can be used with Fish command substitution):

(pkg-config blabla --cflags | posix-to-fish-quotes)

It sounds like there are too many programs in the wild that output posix-quoted so patching them individually to play nice with fish (as considered with pkg-config) wouldn't be easy. Meanwhile, creating wrappers for them on the fish side is problematic because these tools don't always output posix-quoted arguments, so the wrapper would need to be smart and only apply quoting when necessary. In the face of this, having a posix-to-fish conversion function and asking users to pipe the output of pkg-config into it strikes me as a pragmatic and robust solution.

Some people suggested using various one-liners to implement this conversion function (perl, fish string splitting, eval, etc) but those one-liners won't correctly cover corner cases like quoted spaces in the input string or when IFS is set to a different character in Fish. I think that having fish provide an official builtin function for this would let this conversion be implemented correctly in a single place. I understand that asking fish users to call a special function to have pkg-config work right isn't the most discoverable thing but I feel it might be the best we can get. I suppose we could mention it in the documentation for command substitution to help a bit...

@faho

This comment has been minimized.

Copy link
Member

@faho faho commented Apr 30, 2018

those one-liners won't correctly cover corner cases like quoted spaces in the input string

That doesn't actually happen here. In POSIX, command substitutions are just split on $IFS, quotes have no special meaning.

E.g.

$ printf '%s\n' $(echo "a string 'with spaces'")
a
string
'with
spaces'

Note how 'with and spaces' appear on separate lines? That's because they're separate arguments to printf.

when IFS is set to a different character in Fish

I don't think we need to support that, because $IFS would be set on the fish side.

What technically would have to be done is to split runs of the splitting characters, so instead of string split " " | string split \t (because tab is also in POSIX' IFS by default), we'd have to do something like string replace -ra '[ \t]+' \n (because fish splits on newlines, replacing all runs of spaces or tabs with that will work).

I don't think that quite requires a wrapper - with these *-config things, all cases I've seen can be handled via just string split " ", and the issue is more that something needs to be done at all, which means you need to know to do something.

@faho

This comment has been minimized.

Copy link
Member

@faho faho commented Oct 24, 2018

This has now been made an FAQ entry.

It doesn't appear like there's any movement here, so I'm closing this.

@faho faho closed this Oct 24, 2018
@faho faho removed this from the fish-future milestone Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.