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.

Sign up for GitHub

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

Jump to bottom

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

Closed
ejgallego opened this issue Feb 13, 2019 · 16 comments · Fixed by ocaml/opam-repository#13451 or ocaml/opam-repository#13501
Closed

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

ejgallego opened this issue Feb 13, 2019 · 16 comments · Fixed by ocaml/opam-repository#13451 or ocaml/opam-repository#13501
Assignees
@rgrinberg

Comments

@ejgallego
Copy link
Collaborator

New quoting rules in 1.7.0 means that:

module C = Configurator.V1
let _ = C.Pkg_config.query p ~package:"gtk+-3.0 >= 3.10"

will always return None as the package name is quoted in such a way that pkg-config will get confused.

This can be worked around by using:

let _ = C.Pkg_config.query p ~package:"gtk+-3.0>=3.10"

however it is not clear to me that the quoting in 1.7.0 is correct.
@ejgallego
Copy link
Collaborator Author

This may be more serious than what I though, as the below solution seems broken on OSX:

-> stderr:
run: /usr/local/bin/pkg-config gtkspell3-3.0>=3.0.4
Fatal error: exception Sys_error("/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/ocaml-configuratore0f15a/stdout-2: No such file or directory")

@rgrinberg
Copy link
Member

What new quoting rules are you talking about? I can't recall any changes here.

@ejgallego
Copy link
Collaborator Author

Let me locate the commit on src/configurator but indeed it seems that maybe now some commands are being quoted twice.

@ejgallego
Copy link
Collaborator Author

The bisect was a pain for other reasons but indeed seems that a5becc4 creates this problem.

@ejgallego
Copy link
Collaborator Author

Incorrect quoting on Linux is:

run: /usr/bin/pkg-config ''\''gtk+-3.0 >= 3.18'\'''

@rgrinberg
Copy link
Member

Okay, got it. @Chris00 do you mind restoring the old quoting behavior?

@ejgallego ejgallego mentioned this issue Feb 13, 2019
Closed
@ghost
Copy link

ghost commented Feb 13, 2019

I think that just removing the quote_if_needed calls in Pkg_config.query should be enough. The quoting is now handled by Process.run, so things are double quoted at the moment.

@Chris00
Copy link
Member

Chris00 commented Feb 13, 2019

I'll try to take a look to that today.

@rgrinberg
Copy link
Member

#1834 is my attempt to address this

Chris00 added a commit to Chris00/dune that referenced this issue Feb 13, 2019
@Chris00
Fix double quoting of pkg-config when package contains a space
6944111 
Example: Pkg_config.query p ~package:"gtk+-3.0 >= 3.10"

Fixes ocaml#1833

Signed-off-by: Christophe Troestler <Christophe.Troestler@umons.ac.be>
Chris00 added a commit to Chris00/dune that referenced this issue Feb 13, 2019
@Chris00
Fix double quoting of pkg-config when package contains a space
a942ff4 
Example: Pkg_config.query p ~package:"gtk+-3.0 >= 3.10"

Fixes ocaml#1833

Signed-off-by: Christophe Troestler <Christophe.Troestler@umons.ac.be>
@Chris00 Chris00 mentioned this issue Feb 13, 2019
Closed
@Chris00
Copy link
Member

Chris00 commented Feb 13, 2019

@rgrinberg Sorry, I just saw you attempt. (I tested and pushed a fix that is quite similar to yours.)

@Chris00
Copy link
Member

Chris00 commented Feb 13, 2019

The bisect was a pain for other reasons

@ejgallego May you explain a bit more why?

@Chris00
Copy link
Member

Chris00 commented Feb 13, 2019

@rgrinberg I'll also submit an improved doc for query—I think explaining the possibility to put a constraint in the query would be useful for our users.

@ejgallego
Copy link
Collaborator Author

@ejgallego May you explain a bit more why?

Sure, I just did the bisect on a live switch and I got some problems with make install not working, also some trouble due to dune-package.

ejgallego added a commit to ejgallego/lablgtk that referenced this issue Feb 13, 2019
@ejgallego
[build] Conflict with Dune 1.7.0
d9ab3c2 
Bug ocaml/dune#1833 makes our quoting strategy
fail; unfortunately it is not easy to fix due to OS-specific issues.

IMHO the issue is not so serious as to warrant an ugly workaround,
thus we rather declare a conflict with Dune 1.7.0.
@rgrinberg rgrinberg mentioned this issue Feb 13, 2019
Closed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this issue Feb 13, 2019
@rgrinberg
[new release] dune (1.7.1)
14e28cd 
CHANGES:

- Fix the watch mode (ocaml/dune#1837, ocaml/dune#1839, fix ocaml/dune#1836, @diml)

- Configurator: Fix misquoting when running pkg-config (ocaml/dune#1835, fix ocaml/dune#1833,
  @Chris00)
@rgrinberg rgrinberg mentioned this issue Feb 13, 2019
Merged
@ejgallego
Copy link
Collaborator Author

ejgallego commented Feb 13, 2019 •
edited
Loading

Quoting seems still broken on OSX + 1.7.1, will investigate more [no OSX machine at hand]

https://travis-ci.org/garrigue/lablgtk/jobs/492620761

@ejgallego ejgallego reopened this Feb 13, 2019
@rgrinberg
Copy link
Member

Indeed I see the issue - It's related to our janky homebrew support. Fixing.

@rgrinberg rgrinberg self-assigned this Feb 14, 2019
garrigue pushed a commit to garrigue/lablgtk that referenced this issue Feb 15, 2019
@ejgallego @garrigue
[opam] [dune-release] Fix opam metadata to reflect recent release. (#48)
35cf862 
* [build] Conflict with Dune 1.7.0

Bug ocaml/dune#1833 makes our quoting strategy
fail; unfortunately it is not easy to fix due to OS-specific issues.

IMHO the issue is not so serious as to warrant an ugly workaround,
thus we rather declare a conflict with Dune 1.7.0.

* [opam] [dune-release] Fix opam metadata to reflect recent release.

After 3.0.beta4, the loose constraints should mostly work; depending
on the changes we do of course.

We also update the `homepage` field of the OPAM files, and the CHANGES
file, if I understand correctly this should indeed make `dune-release`
work out of the box [and to produce sensible changelogs]

Note that dune-release was incorrectly reading `CHANGES.API` to create
the OPAM changelog! So I had to fix that too.
@ejgallego
Copy link
Collaborator Author

TTBOMK closed by #1842

rgrinberg added a commit to rgrinberg/opam-repository that referenced this issue Feb 21, 2019
@rgrinberg
[new release] dune (1.7.2)
a65d41c 
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 rgrinberg mentioned this issue Feb 21, 2019
Merged
@ejgallego ejgallego mentioned this issue Apr 7, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rgrinberg rgrinberg

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@rgrinberg @Chris00 @ejgallego