Fix pkg-config quoting behavior #1842

rgrinberg · 2019-02-14T04:13:36Z

Shows that on OSX (whenever brew is present), things are not being quoted correctly. This is another incentive for us to drop the builtin homebrew support we have in configurator (cc @avsm, @ejgallego, and @andreypopp)

rgrinberg · 2019-02-14T04:26:18Z

To reiterate the arguments against any kind of homebrew support:

we don't have any special support for any of the package managers out there
the current support does not let users opt out of homebrew easily. They can't use macports for example.

The root of the problem is that we'd like dune to look for external libraries in places that the user has not specified explicitly. I'm not sure it's such a good idea to do this by default, and if it's done at all, it should clearly be more flexible than we have now

rgrinberg · 2019-02-14T05:15:01Z

The last commit removes the homebrew magic, which is admittedly a breaking change and not something we can just do.

A more backwards compatible way to proceed would be to leave the homebrew stuff intact, and parse the ~query the user provides. We basically ignore anything after > < or =.

rgrinberg · 2019-02-14T05:48:57Z

Okay, so I implemented the workaround mentioned above. It's not very attractive but should do the job. For dropping the brew support, perhaps we should finally introduce that V2 API. The API would be the same, but only V1 would use brew. I'm not such a fan of this idea, but I don't think we should break user packages.

Chris00 · 2019-02-14T10:21:37Z

src/configurator/v1.ml

+              | init :: xs ->
+                let pos = List.fold_left ~init ~f:min xs in
+                String.trim (String.take package pos)
+            in
            [sprintf "PKG_CONFIG_PATH=$PKG_CONFIG_PATH:%s/opt/%s/lib/pkgconfig"


Wouldn't it be better to construct %s/opt/%s/lib/pkgconfig and quote it?

Quoting the environment is a good idea (Although I have a better fix for that coming up), but I don't think it would fix the issue. The ~package contains a pkg-config query which contains more than just name + version. So it wouldn't work to include it in the path.

rgrinberg · 2019-02-14T10:35:38Z

Another possibility would be to add another function for our pkg-config API that would take the package name and the version query separately.

rgrinberg · 2019-02-14T17:38:40Z

I've pushed a commit that avoids all env/quoting issues for pkg-config by avoiding the shell in the first place. We could extend its usage in other places, but there's no immediate need. It also seems like we should just extract this code and share it in our spawn module. We have a few other spots with very similar code.

As for the querying issue, @ejgallego I think the best course of action to fix the v1 API is to an optional argument. Roughly:

val query : t -> ?version_expr:string -> package:string -> package_conf option

In the V2 API, we'll get rid of this homebrew support altogether.

ejgallego · 2019-02-14T17:49:19Z

As for the querying issue, @ejgallego I think the best course of action to fix the v1 API is to an optional argument.

I am very far from an expert on pkg-config but this doesn't seem right to me. pkg-config query language is not exactly trivial and users may come up with some complex constraints gtk+3.0 >= 3.12 libxml = 3 thus IMHO the package argument should be properly sent as a single quoted entry.

Also this seems better to keep compatibility in V1.

rgrinberg · 2019-02-14T18:07:11Z

It certainly doesn't seem right to me either. But to "fix" the current homebrew behavior, we need to know the package name. Otherwise, we will not know how to append the PKG_CONFIG_PATH.

How about:

val query' : t -> query:string -> package_name:string -> package_conf option

where you provide the full expression query.

avsm · 2019-02-14T18:15:57Z

@rgrinberg wrote:

The root of the problem is that we'd like dune to look for external libraries in places that the user has not specified explicitly. I'm not sure it's such a good idea to do this by default, and if it's done at all, it should clearly be more flexible than we have now

I agree with this entirely. But we have to bear in mind that macOS developers form the majority of our desktop-dev users, and we should try to make things work out of the box. Some mechanism for Dune to centrally specify a search mechanism that could be tailored for the local package manager in one place would be extremely useful to have. Homebrew is very predictable in where it places libraries -- it's just different from all the other package managers.

ejgallego · 2019-02-14T18:20:53Z

where you provide the full expression query.

I'm afraid I don't know the necessary details to contribute a lot more here :(

I am just wondering why we cannot restore the old behavior, which so far used to work fine, didn't it?

rgrinberg · 2019-02-14T18:23:42Z

I am just wondering why we cannot restore the old behavior, which so far used to work fine, didn't it?

The old behavior worked, but our homebrew support was still broken. By default, homebrew's pkg-config will indeed list most packages (which includes the gtk family of packages), so no homebrew specific hacks are necessary. However, if you tried doing that kind of expression with openssl for example, it would be broken again.

rgrinberg · 2019-02-14T18:26:21Z

I agree with this entirely. But we have to bear in mind that macOS developers form the majority of our desktop-dev users, and we should try to make things work out of the box. Some mechanism for Dune to centrally specify a search mechanism that could be tailored for the local package manager in one place would be extremely useful to have. Homebrew is very predictable in where it places libraries -- it's just different from all the other package managers.

Note that the issue isn't with homebrew in general, but only a few select packages that are "keg only" - openssl being the most prominent example. I think it's acceptable that these packages will take on these hacks rather than configurator.

ejgallego · 2019-02-14T18:29:08Z

I see, thanks for the explanation @rgrinberg

ghost · 2019-02-18T11:44:26Z

Looking at this discussion, I suggest the following changes:

we introduce query' with the suggested API
we keep the quoting performed by query unchanged (i.e. as in 1.6)
to avoid constructing bindings of the form PKG_CONFIG_PATH=/opt/gtk >= 3/lib/pkg-config, we only extend PKG_CONFIG_PATH if the directory /opt/PACKAGE/lib/pkg-config exists, where PACKAGE is the exact string given by the user
we make query display a warning when the package name contains special characters

with this plan, existing packages won't be affected. Packages using configurator to query keg-only pkg-config packages will have to upgrade to query' in order to perform complex queries.

ejgallego · 2019-02-18T12:28:58Z

Isn't it query' a bit of an strange name?

ghost · 2019-02-18T13:50:18Z

Indeed. I don't have a better suggestion though

rgrinberg · 2019-02-18T18:35:05Z

I've added a query_expr function that attempts to implement the proposal. @ejgallego could you try it out?

rgrinberg · 2019-02-19T10:33:12Z

@diml this is ready for review

src/configurator/v1.ml

ghost · 2019-02-19T10:51:04Z

src/configurator/v1.ml

+        let stdout = openfile stdout_fn in
+        let stderr = openfile stderr_fn in
+        let (stdin, stdin_w) = Unix.pipe () in
+        Unix.close stdin_w;


No objection here, but note that we don't do this for processes started by dune

ejgallego · 2019-02-19T17:04:51Z

I've added a query_expr function that attempts to implement the proposal. @ejgallego could you try it out?

Thanks @rgrinberg , I've launched a build with this branch here:

https://travis-ci.org/ejgallego/lablgtk/builds/495552827

it seems to work !

And this is one where I am using the new API:

https://travis-ci.org/ejgallego/lablgtk/builds/495570774

one build fails due to missing OPAM constraint on dune >= 1.7.2 but indeed it seems to work fine too.

Shows that on OSX (whenever brew is present), things are not being quoted correctly. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Running it directly avoids issues with quoting and environment handling Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

For more complex pkg config queries Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

@diml

CHANGES: - Add `${corrected-suffix}`, `${library-name}` and a few other variables to the list of variables to upgrade. This fixes the support for various framework producing corrections (ocaml/dune#1840, ocaml/dune#1853, @diml) - Fix `$ dune subst` failing because the build directory wasn't set. (ocaml/dune#1854, fix ocaml/dune#1846, @rgrinberg) - Configurator: Add warning to `Pkg_config.query` when a full package expression is used. Add `Pkg_config.query_expr` for cases when the full power of pkg-config's querying is needed (ocaml/dune#1842, fix ocaml/dune#1833, @rgrinberg) - Fix unavailable, optional implementations eagerly breaking the build (ocaml/dune#1857, fix ocaml/dune#1856, @rgrinberg)

rgrinberg requested a review from a user February 14, 2019 04:13

rgrinberg force-pushed the show-quoting-issue-osx branch from f3c9b8f to 8760fa4 Compare February 14, 2019 04:14

rgrinberg force-pushed the show-quoting-issue-osx branch from c7cec74 to 4711c30 Compare February 14, 2019 05:39

Chris00 reviewed Feb 14, 2019

View reviewed changes

rgrinberg force-pushed the show-quoting-issue-osx branch from 10f7318 to d66e721 Compare February 14, 2019 10:26

rgrinberg requested a review from emillon as a code owner February 14, 2019 10:26

rgrinberg changed the title ~~Add test for pkg-config arugment quoting~~ Add test for pkg-config argument quoting Feb 14, 2019

rgrinberg changed the title ~~Add test for pkg-config argument quoting~~ Fix pkg-config quoting behavior Feb 17, 2019

rgrinberg force-pushed the show-quoting-issue-osx branch from 1c04313 to 4cc4218 Compare February 18, 2019 18:34

rgrinberg force-pushed the show-quoting-issue-osx branch 2 times, most recently from c6c9b04 to 8aabc1b Compare February 19, 2019 10:31

ghost suggested changes Feb 19, 2019

View reviewed changes

src/configurator/v1.ml Outdated Show resolved Hide resolved

ghost reviewed Feb 19, 2019

View reviewed changes

rgrinberg force-pushed the show-quoting-issue-osx branch from 70972e8 to 8ee0fbc Compare February 20, 2019 01:40

ghost approved these changes Feb 20, 2019

View reviewed changes

rgrinberg added 4 commits February 20, 2019 16:03

Add test for pkg-config arugment quoting

ffcf443

Shows that on OSX (whenever brew is present), things are not being quoted correctly. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Run pkg-config process without shell

7ceea79

Running it directly avoids issues with quoting and environment handling Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Implement a Pkg_config.query_expr function

0e8655f

For more complex pkg config queries Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

update CHANGES

666b6b6

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg force-pushed the show-quoting-issue-osx branch from 8ee0fbc to 666b6b6 Compare February 20, 2019 09:03

rgrinberg merged commit 8fc5284 into ocaml:master Feb 20, 2019

rgrinberg deleted the show-quoting-issue-osx branch February 20, 2019 09:14

ejgallego mentioned this pull request Feb 20, 2019

[configurator] New quoting rules may create problems. #1833

Closed

rgrinberg mentioned this pull request Feb 21, 2019

[new release] dune (1.7.2) ocaml/opam-repository#13501

Merged

This was referenced Apr 8, 2019

More general support for installation of coqide keys coq/coq#8567

Merged

Changes in dune >= 1.7.2 break reporting pkg-config errors in lablgtk3 beta5 on macosx garrigue/lablgtk#59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pkg-config quoting behavior #1842

Fix pkg-config quoting behavior #1842

rgrinberg commented Feb 14, 2019 •

edited

Loading

rgrinberg commented Feb 14, 2019

rgrinberg commented Feb 14, 2019

rgrinberg commented Feb 14, 2019

Chris00 Feb 14, 2019

rgrinberg Feb 14, 2019

rgrinberg commented Feb 14, 2019

rgrinberg commented Feb 14, 2019

ejgallego commented Feb 14, 2019 •

edited

Loading

rgrinberg commented Feb 14, 2019

avsm commented Feb 14, 2019

ejgallego commented Feb 14, 2019

rgrinberg commented Feb 14, 2019 •

edited

Loading

rgrinberg commented Feb 14, 2019

ejgallego commented Feb 14, 2019

ghost commented Feb 18, 2019

ejgallego commented Feb 18, 2019 •

edited

Loading

ghost commented Feb 18, 2019

rgrinberg commented Feb 18, 2019

rgrinberg commented Feb 19, 2019

ghost Feb 19, 2019 •

edited by ghost

Loading

ejgallego commented Feb 19, 2019 •

edited

Loading

Fix pkg-config quoting behavior #1842

Fix pkg-config quoting behavior #1842

Conversation

rgrinberg commented Feb 14, 2019 • edited Loading

rgrinberg commented Feb 14, 2019

rgrinberg commented Feb 14, 2019

rgrinberg commented Feb 14, 2019

Chris00 Feb 14, 2019

Choose a reason for hiding this comment

rgrinberg Feb 14, 2019

Choose a reason for hiding this comment

rgrinberg commented Feb 14, 2019

rgrinberg commented Feb 14, 2019

ejgallego commented Feb 14, 2019 • edited Loading

rgrinberg commented Feb 14, 2019

avsm commented Feb 14, 2019

ejgallego commented Feb 14, 2019

rgrinberg commented Feb 14, 2019 • edited Loading

rgrinberg commented Feb 14, 2019

ejgallego commented Feb 14, 2019

ghost commented Feb 18, 2019

ejgallego commented Feb 18, 2019 • edited Loading

ghost commented Feb 18, 2019

rgrinberg commented Feb 18, 2019

rgrinberg commented Feb 19, 2019

ghost Feb 19, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ejgallego commented Feb 19, 2019 • edited Loading

rgrinberg commented Feb 14, 2019 •

edited

Loading

ejgallego commented Feb 14, 2019 •

edited

Loading

rgrinberg commented Feb 14, 2019 •

edited

Loading

ejgallego commented Feb 18, 2019 •

edited

Loading

ghost Feb 19, 2019 •

edited by ghost

Loading

ejgallego commented Feb 19, 2019 •

edited

Loading