haskell / cabal

Official upstream development repository for Cabal and cabal-install
https://haskell.org/cabal
Other
1.62k stars 697 forks source link

pkg-config output parser doesn't implement shell quoting rules #5519

Open VitorCBSB opened 6 years ago

VitorCBSB commented 6 years ago

Title is my main suspicion of the cause of the issue:

So, I've been trying to build the SDL2 package in Windows 10 and made an issue about the problem I've been having around a year ago or so. If you don't want to read it, it basically cannot find my pacman-installed SDL2 for some reason, even though the directories that include its lib and include files are there in the extra-lib-dirs and extra-include-dirs arguments respectively. Because my home directory has a space character in it, basically every single directory's absolute path is also going to have it, so I have no easy way to test the removal or addition of spaces. So, for example, like how we can see in the linked issue, the configure command is being passed --extra-lib-dirs=C:\\Users\\Vitor Coimbra\\AppData\\Local\\Programs\\stack\\x86_64-windows\\msys2-20150512\\mingw64\\lib, which is the directory that contains, among other things, my libSDL2.a file.

What I'm certain of is that it has something to do with the pkgconfig-depends field, because another project that uses foreign dependencies (namely, taglib) and does not use that particular field (instead, opting to use an extra-libraries one) builds correctly in my machine. If I change its Cabal file to use a pkgconfig-depends: taglib field instead, it stops building with the same kind of error seen in the SDL2's issue (but for taglib, obviously). To be more specific, I'm using the stack-installed MSYS2 program to manage system packages like pkg-config, sdl2 and taglib with pacman.

Poking around Cabal's code, my first intuition was to find the place where the error message was being generated. There, one of the problems I thought might be happening are in these two places:

https://github.com/haskell/cabal/blob/fc9a60795bb5a655cc72dc1f493a29db7b05019f/Cabal/Distribution/Simple/Configure.hs#L1744-L1750

https://github.com/haskell/cabal/blob/fc9a60795bb5a655cc72dc1f493a29db7b05019f/Cabal/Distribution/Simple/Configure.hs#L1727-L1733

See, because of the way they create the -I and -L values, it seems to me they're not properly treating directories with characters such as space correctly. This might not be actually be the problem, though, because of the way processes are created, using a list of arguments instead of concatenating it all into one huge string, so this might not be a good lead. Plus, the fact that it works for htaglib shows that it's probably not here.

hvr commented 6 years ago

Does this reproduce with cabal as well?

If it does, can you provide us the output of

?

VitorCBSB commented 6 years ago

Unfortunately, I have no idea how to even begin trying to reproduce this with Cabal, as I've never used it, so I appreciate any guidance anyone may have. In any case, I'll try running a few different things. From what I can tell, since it seems to be happening while running cabal configure (called by stack), I'll try that first when I get home tonight.

hvr commented 6 years ago

@VitorCBSB Some intro material for cabal: See https://hub.zhox.com/posts/chocolatey-introduction/ and maybe https://haskell-at-work.com/episodes/2018-05-13-introduction-to-cabal.html

VitorCBSB commented 6 years ago

@hvr Thanks, that was very helpful.

From what I gather, configure, build, install and other commands are considered deprecated in favor of their new-* counterparts?

Running the new-build command as cabal new-build "--with-ghc=C:\\Users\\Vitor Coimbra\\AppData\\Local\\Programs\\stack\\x86_64-windows\\ghc-8.0.2\\bin\\ghc.EXE" unfortunately seems to fail for a different reason:

C:\Users\Vitor Coimbra\sdl2>cabal new-build "--with-ghc=C:\\Users\\Vitor Coimbra\\AppData\\Local\\Programs\\stack\\x86_64-windows\\ghc-8.0.2\\bin\\ghc.EXE"
Build profile: -w ghc-8.0.2 -O1
In order, the following will be built (use -v for more details):
 - distributive-0.6 (lib:distributive) (requires build)
 - hashable-1.2.7.0 (lib) (requires build)
 - comonad-5.0.4 (lib:comonad) (requires build)
 - unordered-containers-0.2.9.0 (lib) (requires build)
 - scientific-0.3.6.2 (lib) (requires build)
 - bifunctors-5.5.3 (lib) (requires build)
 - bytes-0.15.5 (lib:bytes) (requires build)
 - semigroupoids-5.3.1 (lib:semigroupoids) (requires build)
 - profunctors-5.3 (lib) (requires build)
 - invariant-0.5.1 (lib) (requires build)
 - free-5.1 (lib) (requires build)
 - adjunctions-4.4 (lib) (requires build)
 - kan-extensions-5.2 (lib:kan-extensions) (requires build)
 - lens-4.17 (lib:lens) (requires build)
 - linear-1.20.8 (lib:linear) (requires build)
 - sdl2-2.4.1.0 (lib) (first run)
Configuring distributive-0.6 (all, legacy fallback)...
Configuring hashable-1.2.7.0 (lib)...
Building hashable-1.2.7.0 (lib)...
Configuring scientific-0.3.6.2 (lib)...
Configuring unordered-containers-0.2.9.0 (lib)...
Building scientific-0.3.6.2 (lib)...
Building unordered-containers-0.2.9.0 (lib)...
Building distributive-0.6 (all, legacy fallback)...

Failed to build distributive-0.6. The failure occurred during the final
install step.
Build log ( C:\Users\Vitor
Coimbra\AppData\Roaming\cabal\logs\ghc-8.0.2\distributive-0.6-a40629d1ee1fb3fb758eb20738eccabd92c7f25b.log
):
Installing library in C:\Users\Vitor
Coimbra\AppData\Roaming\cabal\store\ghc-8.0.2\incoming\new-14708\Users\Vitor
Coimbra\AppData\Roaming\cabal\store\ghc-8.0.2\distributive-0.6-a40629d1ee1fb3fb758eb20738eccabd92c7f25b\lib
copyFile: does not exist (O sistema não pode encontrar o caminho
especificado.)
dist\setup\setup.exe ...
Warning: distributive.cabal: Unknown fields: build-tool-depends (line 107)
Fields allowed in this section:
type, main-is, test-module, buildable, build-tools, build-depends,
cpp-options, cc-options, ld-options, pkgconfig-depends, frameworks,
extra-framework-dirs, c-sources, js-sources, default-language,
other-languages, default-extensions, other-extensions, extensions,
extra-libraries, extra-ghci-libraries, extra-lib-dirs, includes,
install-includes, include-dirs, hs-source-dirs, other-modules,
ghc-prof-options, ghcjs-prof-options, ghc-shared-options,
ghcjs-shared-options, ghc-options, ghcjs-options, jhc-options,
hugs-options, nhc98-options
Configuring distributive-0.6...
cabal: Failed to build distributive-0.6 (which is required by sdl2-2.4.1.0).
See the build log above for details.

And I know you only asked for the pkg-config outputs only if I were able to build sdl2 with cabal, but here it is anyway.

For sdl2:

C:\Users\Vitor Coimbra\sdl2>pkg-config --debug --cflags --libs sdl2
Error printing enabled by default due to use of output options besides --exists, --atleast/exact/max-version or --list-all. Value of --silence-errors: 0
Error printing enabled
Adding virtual 'pkg-config' package to list of known packages
Looking for package 'sdl2'
Looking for package 'sdl2-uninstalled'
Reading 'sdl2' from file 'C:\Users\Vitor Coimbra\AppData\Local\Programs\stack\x86_64-windows\msys2-20150512\mingw64\lib\pkgconfig\sdl2.pc'
Parsing package file 'C:\Users\Vitor Coimbra\AppData\Local\Programs\stack\x86_64-windows\msys2-20150512\mingw64\lib\pkgconfig\sdl2.pc'
  line>
  line>
  line>prefix=/mingw64
 Variable declaration, 'prefix' overridden with 'C:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64'
  line>exec_prefix=${prefix}
 Variable declaration, 'exec_prefix' has value 'C:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64'
  line>libdir=${exec_prefix}/lib
 Variable declaration, 'libdir' has value 'C:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib'
  line>includedir=${prefix}/include
 Variable declaration, 'includedir' has value 'C:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include'
  line>
  line>Name: sdl2
  line>Description: Simple DirectMedia Layer is a cross-platform multimedia library designed to provide low level access to audio, keyboard, mouse, joystick, 3D hardware via OpenGL, and 2D video framebuffer.
  line>Version: 2.0.8
  line>Requires:
  line>Conflicts:
  line>Libs: -L${libdir}  -lmingw32 -lSDL2main -lSDL2  -mwindows
  line>Libs.private: -lmingw32 -lSDL2main -lSDL2  -mwindows  -Wl,--no-undefined -lm -ldinput8 -ldxguid -ldxerr8 -luser32 -lgdi32 -lwinmm -limm32 -lole32 -loleaut32 -lshell32 -lversion -luuid -static-libgcc
Unknown keyword 'Libs.private' in 'C:\Users\Vitor Coimbra\AppData\Local\Programs\stack\x86_64-windows\msys2-20150512\mingw64\lib\pkgconfig\sdl2.pc'
  line>Cflags: -I${includedir}/SDL2  -Dmain=SDL_main
Path position of 'sdl2' is 1
Adding 'sdl2' to list of known packages
 post-recurse: sdl2
adding CFLAGS_OTHER string "-Dmain=SDL_main "
 post-recurse: sdl2
 original: sdl2
   sorted: sdl2
adding CFLAGS_I string "-IC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include/SDL2 "
 post-recurse: sdl2
 original: sdl2
   sorted: sdl2
adding LIBS_L string "-LC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib "
 post-recurse: sdl2
 removing duplicate "-lSDL2main"
 removing duplicate "-lSDL2"
 removing duplicate "-mwindows"
adding LIBS_OTHER | LIBS_l string "-lmingw32 -lSDL2main -lSDL2 -mwindows "
returning flags string "-Dmain=SDL_main -IC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include/SDL2 -LC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib -lmingw32 -lSDL2main -lSDL2 -mwindows"
-Dmain=SDL_main -IC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include/SDL2 -LC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib -lmingw32 -lSDL2main -lSDL2 -mwindows

And taglib:

C:\Users\Vitor Coimbra\sdl2>pkg-config --debug --cflags --libs taglib
Error printing enabled by default due to use of output options besides --exists, --atleast/exact/max-version or --list-all. Value of --silence-errors: 0
Error printing enabled
Adding virtual 'pkg-config' package to list of known packages
Looking for package 'taglib'
Looking for package 'taglib-uninstalled'
Reading 'taglib' from file 'C:\Users\Vitor Coimbra\AppData\Local\Programs\stack\x86_64-windows\msys2-20150512\mingw64\lib\pkgconfig\taglib.pc'
Parsing package file 'C:\Users\Vitor Coimbra\AppData\Local\Programs\stack\x86_64-windows\msys2-20150512\mingw64\lib\pkgconfig\taglib.pc'
  line>prefix=/mingw64
 Variable declaration, 'prefix' overridden with 'C:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64'
  line>exec_prefix=/mingw64
 Variable declaration, 'exec_prefix' has value '/mingw64'
  line>libdir=/mingw64/lib
 Variable declaration, 'libdir' has value 'C:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib'
  line>includedir=/mingw64/include
 Variable declaration, 'includedir' has value 'C:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include'
  line>
  line>Name: TagLib
  line>Description: Audio meta-data library
  line>Requires:
  line>Version: 1.11.1
  line>Libs: -L${libdir} -ltag
  line>Cflags: -I${includedir}/taglib
Path position of 'taglib' is 1
Adding 'taglib' to list of known packages
 post-recurse: taglib
adding CFLAGS_OTHER string ""
 post-recurse: taglib
 original: taglib
   sorted: taglib
adding CFLAGS_I string "-IC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include/taglib "
 post-recurse: taglib
 original: taglib
   sorted: taglib
adding LIBS_L string "-LC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib "
 post-recurse: taglib
adding LIBS_OTHER | LIBS_l string "-ltag "
returning flags string "-IC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include/taglib -LC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib -ltag"
-IC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include/taglib -LC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib -ltag
VitorCBSB commented 6 years ago

Sorry for the double post. I created an empty project with a dependency on taglib (via the pkgconfig-depends field) and got the same problem as the original issue:

C:\Users\Vitor Coimbra\Wowee>cabal new-build "--with-ghc=C:\\Users\\Vitor Coimbra\\AppData\\Local\\Programs\\stack\\x86_64-windows\\ghc-8.0.2\\bin\\ghc.EXE"
Resolving dependencies...
Build profile: -w ghc-8.0.2 -O1
In order, the following will be built (use -v for more details):
 - Wowee-0.1.0.0 (lib) (first run)
 - Wowee-0.1.0.0 (exe:Wowee-exe) (first run)
Configuring library for Wowee-0.1.0.0..
cabal.exe: Missing dependency on a foreign library:
* Missing (or bad) C library: tag
This problem can usually be solved by installing the system package that
provides this library (you may need the "-dev" version). If the library is
already installed but in a non-standard location then you can use the flags
--extra-include-dirs= and --extra-lib-dirs= to specify where it is.If the
library file does exist, it may contain errors that are caught by the C
compiler at the preprocessing stage. In this case you can re-run configure
with the verbosity flag -v3 to see the error messages.

cabal: Failed to build Wowee-0.1.0.0 (which is required by exe:Wowee-exe from
Wowee-0.1.0.0). The failure occurred during the configure step.
hvr commented 6 years ago

@VitorCBSB

From what I gather, configure, build, install and other commands are considered deprecated in favor of their new-* counterparts?

yes; they have been in development since 2015; so it's about time we start switching over :-)

Running the new-build command as cabal new-build... unfortunately seems to fail for a different reason:

..that's most likely hitting windows' maximum filepath limitation; try setting an explicit shorter store path with cabal new-build --store-dir=C:\\sr\ new-build ... (we should probably emit a warning on windows for that; see first blogpost I linked for an explanation of the --store-dir workaround)

...as for the pkg-config output: I'm afraid the pkg-config output is broken; this wouldn't only break cabal but every other build-tool relying on pkg-config

So, you'll have to repair the .pc files sdl2.pc and taglib.pc if you want this to work.

VitorCBSB commented 6 years ago

I'll try enabling long paths tonight and see if that makes the build work.

I'm afraid the pkg-config output is broken; this wouldn't only break cabal but every other build-tool relying on pkg-config

Oh, so the culprit is actually pkg-config? I'm not sure how I would go about fixing their .pc files. How does it differ from the expected output?

hvr commented 6 years ago

NOTE: I just realised the comment below may not be accurate


@VitorCBSB it's not properly quoted:

Instead of

-IC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include/taglib -LC:/Users/Vitor\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib -ltag

it should have emitted

"-IC:/Users/Vitor Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/include/taglib" "-LC:/Users/Vitor Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib" -ltag

for tooling to be able to recognize which arguments are supposed to contain whitespaces and where the whitespace is a separator.

This could be a bug in the way Stack provisions msys2 or even an upstream bug in msys2. But in any case there's nothing sensible we can do in cabal; therefore I'm closing this ticket for now; feel free to continue discussiing/commenting here if you have additional information to add, or any further questions!

hvr commented 6 years ago

@VitorCBSB I need to investigate a bit more; the pkg-config output may after all be correct if the \-escaping is valid; so far I was only used to pkg-config using ""-style quoting for whitespaces and never noticed \ might be valid too; although I'm wondering how this would interact w/ windows-style paths when they use \ as a path-separator (i.e. C:\Users\...)

VitorCBSB commented 6 years ago

Alright, I'll keep looking around. As far as I can see, these are all the places pkg-config seems to be called, I may have missed some as I feel GitHub's search is a bit unreliable:

https://github.com/haskell/cabal/blob/e2cacc17dad44a2857c632d9432c1b6aad968257/cabal-install/Distribution/Solver/Types/PkgConfigDb.hs#L60-L66

https://github.com/haskell/cabal/blob/e2cacc17dad44a2857c632d9432c1b6aad968257/cabal-install/Distribution/Solver/Types/PkgConfigDb.hs#L147-L151

And in this function:

https://github.com/haskell/cabal/blob/fc9a60795bb5a655cc72dc1f493a29db7b05019f/Cabal/Distribution/Simple/Configure.hs#L1481

hvr commented 6 years ago

@VitorCBSB Currently I suspect that we're not implementing full de-escaping/de-quoting of pkg-config's supported syntax;

The best reference of the pkg-config format I could find is https://dev.gentoo.org/~mgorny/pkg-config-spec.html#cflags and it basically says that

The Cflags keyword contains compiler flags, in a form suitable for passing to a POSIX shell (excluding the shell expansions). When parsing the variable, pkg-config uses shell quoting rules to split it into consecutive command-line arguments.

and

The Libs keyword contains compiler linking flags, in a form suitable for passing to a POSIX shell (excluding the shell expansions). When parsing the variable, pkg-config uses shell quoting rules to split it into consecutive command-line arguments.

IOW, we need to make sure we implement "shell quoting rules" the same way pkg-config does while also taking into account the data encoding rules for \s from https://dev.gentoo.org/~mgorny/pkg-config-spec.html#data-encoding (actually we don't, as we're not parsing the .pc files but rather the output of pkg-config)

VitorCBSB commented 6 years ago

Alright, so investigating a little further, I believe this is where the trouble begins:

https://github.com/haskell/cabal/blob/fc9a60795bb5a655cc72dc1f493a29db7b05019f/Cabal/Distribution/Simple/Configure.hs#L1552-L1558

More specifically, it seems the outputs of pkg-config --cflags and pkg-config --libs are being parsed with words, which doesn't behave correctly when space characters that aren't meant to be used as separators are involved:

Prelude> readFile "libsOutput.txt"
"-LC:/Users/Vitor\\ Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib -ltag\n"
Prelude> words <$> (readFile "libsOutput.txt")
["-LC:/Users/Vitor\\","Coimbra/AppData/Local/Programs/stack/x86_64-windows/msys2-20150512/mingw64/lib","-ltag"]
Prelude>
hvr commented 6 years ago

@VitorCBSB yes, that's exactly the place; we need to replace words with a parser that is able to decode into arguments according to "POSIX shell quoting rules" (iow, supports single quotes, double quotes and backslash-escaping)

VitorCBSB commented 6 years ago

I've been looking around for something capable of that and this library seems like a good candidate. I'm not sure how a project like cabal feels about dependencies, especially for something as specific as this though.

Mistuke commented 6 years ago

I've been looking around for something capable of that and this library seems like a good candidate. I'm not sure how a project like cabal feels about dependencies, especially for something as specific as this though.

I'm not a Cabal maintainer, but that library seems like a no-go to me. It's dependency on megaparsec pulls in a host of other dependencies. Cabal is used during GHC bootstrapping and so these dependencies would be a huge burden for us.

hvr commented 6 years ago

@VitorCBSB there's no need to depend on an external library for a package whose source code (see http://hackage.haskell.org/package/shellwords-0.1.2.1/docs/src/ShellWords.html) could trivially be ported/reimplemented for to Parsec and integrated into cabal's codebase