[configurator] Return pkg-config error to clients of new query API. #1886

ejgallego · 2019-02-27T00:24:51Z

It is very convenient for configuration tools to be able to provide a
better error message to the user when pkg-config fails.

This was suggested by @garrigue.

We take advantage of the newly [undocumented] API in 1.7.2 to modify
its return type, but this could be a problem. If so, we will have to
introduce yet another function if we want this functionality.

ejgallego · 2019-02-27T00:27:51Z

src/configurator/v1.mli

+  val query_expr : t
+    -> package:string
+    -> expr:string
+    -> (package_conf, string) Stdune.Either.t


Umm, exposing Stdune this in this interface file may be a problem right?

Yes, that's definitely not acceptable. Why don't you just return the result type? This will mean that this will require the result compatibility package in configurator, but I think that's acceptable.

Umm, how should such package be exported without exposing Stdune?

You did exactly what I meant. The result type in stdune matches the one in stdlib so there's no need to export it from stdune.

rgrinberg · 2019-02-27T04:23:52Z

Are we allowed to make a breaking change like this to query_expr? I think we probably are as you're the only consumer, but we need to double check somehow.

ejgallego · 2019-02-27T13:06:58Z

Are we allowed to make a breaking change like this to query_expr? I think we probably are as you're the only consumer, but we need to double check somehow.

I guess a way to check is to see if there are any dune >= 1.7.2 dependencies in OPAM.

ghost · 2019-02-27T13:23:35Z

BTW, in general we should a bit be more strict about minor releases. I.e. we shouldn't introduce new functionalities in minor releases, just to give new functionalities a bit more time to mature in master.

rgrinberg · 2019-02-27T16:04:30Z

BTW, in general we should a bit be more strict about minor releases. I.e. we shouldn't introduce new functionalities in minor releases, just to give new functionalities a bit more time to mature in master.

Yeah, that was definitely a mistake. I originally framed this issue as a "regression" so the new API was supposed to be a fix. I'll be more careful.

rgrinberg · 2019-02-27T16:33:17Z

@ejgallego you're getting a compilation error because of a bug on our end. Because we don't have transitive dependencies anymore, Dune_caml's cmi's don't get included.

Just apply this patch:

diff --git a/src/configurator/dune b/src/configurator/dune
index 0507edec..697f264d 100644
--- a/src/configurator/dune
+++ b/src/configurator/dune
@@ -3,5 +3,5 @@
 (library
  (name        configurator)
  (public_name dune.configurator)
- (libraries   stdune ocaml_config dune_lang)
+ (libraries   stdune ocaml_config dune_lang dune_caml)
  (flags       (:standard -safe-string (:include flags/flags.sexp))))

And things should be fixed. In fact, we should depend on dune_caml anywhere we depend on stdune. @diml do we need to think of a solution for re-exporting libraries? I believe @bobot brought this issue up before.

ejgallego · 2019-02-27T17:05:08Z

@ejgallego you're getting a compilation error because of a bug on our end. Because we don't have transitive dependencies anymore, Dune_caml's cmi's don't get included.

Ok, I see thanks! I was indeed puzzled by the error, but now I understand it.

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

Ok, I see thanks! I was indeed puzzled by the error, but now I understand it.

This doesn't work in 4.02 [as expected] so which compat module should we use for it? Should we open Result?

rgrinberg · 2019-02-27T17:25:42Z

I think you can just use Dune_caml.result directly in the mli. Pretty sure we can make Dune_caml public as it doesn't actually add new api's. But let's see what @diml says.

ejgallego · 2019-02-27T17:36:16Z

I think you can just use Dune_caml.result directly in the mli. Pretty sure we can make Dune_caml public as it doesn't actually add new api's. But let's see what @diml says.

Let's go for it to try to get a full CI build.

ghost · 2019-02-27T17:58:22Z

Hmm, I'm not a big fan of publicising Dune_caml given that it is not complete (it doesn't export all the stdlib modules) and we will get rid of it once we drop support for OCaml < 4.07. I suggest to put the result stuff in its own dune.result library, or just define a new result type in configurator. The latter is probably the simplest solution.

ejgallego · 2019-02-27T18:15:36Z

The latter is probably the simplest solution.

Indeed that seems to make the most sense, I've added a result type that can be deprecated and made into alias once 4.02 support is gone.

I did grep for all the dependencies on dune in opam-repository and indeed there is no package depending on dune >= 1.7.2 that I could find, so tweaking this bit of API IMHO can be done, tho personally I would just do another 1.7 release removing the newly-added API and defer this new function to 1.8

rgrinberg · 2019-02-28T05:30:51Z

tho personally I would just do another 1.7 release removing the newly-added API and defer this new function to 1.8

This seems like a case where the cure is worse than the disease. Users might easily introduce issues to their code by having it work against 1.7.2.

There's nothing fundamentally wrong with the old function, perhaps we can just think of a new name for the function that returns a result? query_expr_err sounds find to me for example. Then we can deprecate the old function if it's really necessary. Although I don't see such a strong need as it doesn't introduce any problems.

rgrinberg · 2019-02-28T05:35:53Z

Hmm, I'm not a big fan of publicising Dune_caml given that it is not complete (it doesn't export all the stdlib modules) and we will get rid of it once we drop support for OCaml < 4.07. I suggest to put the result stuff in its own dune.result library, or just define a new result type in configurator. The latter is probably the simplest solution.

Well we'll get rid of dune.result even earlier as it will not be useful once we drop support for 4.02.3.

In any case, I like the dune.result idea. Will implement it in a separate PR.

rgrinberg · 2019-02-28T07:21:18Z

This pr now needs #1892

ejgallego · 2019-02-28T12:55:55Z

There's nothing fundamentally wrong with the old function, perhaps we can just think of a new name for the function that returns a result? query_expr_err sounds find to me for example. Then we can deprecate the old function if it's really necessary. Although I don't see such a strong need as it doesn't introduce any problems.

Sure, I'll rename and add another function.

IMO I would deprecate the old one right away, no reason to keep the API creep.

This pr now needs #1892

Cool, I'll wait for it to be merged.

ejgallego · 2019-02-28T13:13:26Z

Umm, I am not sure I understand how to use the new lib.

rgrinberg · 2019-02-28T13:17:10Z

I believe that this is all you need:

diff --git a/src/configurator/dune b/src/configurator/dune
index c122bdbc..14efb6fc 100644
--- a/src/configurator/dune
+++ b/src/configurator/dune
@@ -3,5 +3,5 @@
 (library
  (name        configurator)
  (public_name dune.configurator)
- (libraries   stdune ocaml_config dune_lang dune._result)
+ (libraries   stdune ocaml_config dune_lang dune_caml dune._result)
  (flags       (:standard -safe-string (:include flags/flags.sexp))))
diff --git a/src/configurator/v1.mli b/src/configurator/v1.mli
index b56ffcff..cadae2fc 100644
--- a/src/configurator/v1.mli
+++ b/src/configurator/v1.mli
@@ -1,3 +1,5 @@
+open! Dune_result
+
 type t
 
 val create

ejgallego · 2019-02-28T13:19:46Z

(libraries stdune ocaml_config dune_lang dune_caml dune._result)

Oh indeed thanks, I was missing the dune_caml lib as for the compiler to be able to spot the transitive equality on types.

rgrinberg

Looks good. Just one minor nit. We prefer to keep all the general purpose functions in stdune.

rgrinberg · 2019-02-28T13:25:33Z

src/configurator/v1.ml

-      None
+      Error stderr
+
+  let res_forget = function Ok p -> Some p | Error _ -> None


This is a standard function, so let's just add this to Stdune.Result. In base, this is called Or_error.ok, but that's probably too cute. Let's just call it Result.to_option or something.

Indeed; I didn't add this to stdune I guess to keep it 1:1 to OCaml's result, but that's the wrong reason. Do you want me to update it on this PR or would you like it to introduce the change in a separate one?

In stdune we extend the stdlib freely btw. We don't even bother to maintain compatibility. Dune_caml is the 1:1 "namespaced" clone, which we only need because Stdlib is >= 4.07. Feel free to just do it here, in a new PR, or just push to master. This is a very a minor change.

Ok will do it here as this PR is the first consumer of such a function.

ejgallego · 2019-03-04T15:40:56Z

Review re-requested; main point to discuss could be the deprecation of the 1.7.2 introduced function, otherwise this should be OK I think.

rgrinberg · 2019-03-04T18:24:04Z

The deprecation is acceptable to me.

@garrigue

It is very convenient for configuration tools to be able to provide a better error message to the user when pkg-config fails. This was suggested by @garrigue. We take advantage of the newly [undocumented] API in 1.7.2 to modify its return type, but this could be a problem. If so, we will have to introduce yet another function if we want this functionality. We also update `src/configurator/dune` to include transitive dependencies as suggested by @rgrinberg Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>

emillon

I don't think we have to deprecate the function right away, but I'm fine if we do.

ejgallego · 2019-03-05T10:52:29Z

I don't think we have to deprecate the function right away, but I'm fine if we do.

It is not used by anyone yet, so IMO it is better if we just try to avoid people use it at all as to have a more compact API.

By the way, maybe at some point it would be useful for dune to include some kind of "dune-config" script? For example https://github.com/garrigue/lablgtk/blob/lablgtk3/src/dune_config.ml

This allows people to write:

(rule
 (targets
   cflag-gtk+-3.0.sexp
   clink-gtk+-3.0.sexp)
 (action (run ./dune_config.exe -pkg gtk+-3.0 -version 3.18)))
...
 (c_flags         (:include cflag-gtk+-3.0.sexp))
 (c_library_flags (:include clink-gtk+-3.0.sexp))

it seems to me that this is enough of a standard pattern as to avoid having N config-like scripts.

ghost · 2019-03-05T11:57:18Z

At this point we might as well add a (pkg_config gtk+-3.0) field

ejgallego · 2019-03-05T12:05:08Z

At this point we might as well add a (pkg_config gtk+-3.0) field

IMO that'd be a very nice solution and cover a lot of cases, let me open a ticket for it.

rgrinberg · 2019-03-07T05:54:42Z

@ejgallego btw, we usually merge our own PR's after approval. So if you're waiting for someone else to merge it, it's unlikely to happen.

ejgallego · 2019-03-07T05:56:30Z

Indeed, sorry. I'm used to the coq model where you never merge your own PRs.

rgrinberg · 2019-03-07T13:20:31Z

I merged this myself to unblock 1.8

@aalekseyev

CHANGES: - Clean up watch mode polling loop: improves signal handling and error handling during polling (ocaml/dune#1912, fix ocaml/dune#1907, fix ocaml/dune#1671, @aalekseyev) - Change status messages during polling to be one-line, so that the messages are correctly erased by ^K. (ocaml/dune#1912, @aalekseyev) - Add support for `.cxx` extension for C++ stubs (ocaml/dune#1831, @rgrinberg) - Add `DUNE_WORKSPACE` variable. This variable is equivalent to setting `--workspace` in the command line. (ocaml/dune#1711, fix ocaml/dune#1503, @rgrinberg) - Add `c_flags` and `cxx_flags` to env profile settings (ocaml/dune#1700 and ocaml/dune#1800, @gretay-js) - Format `dune printenv` output (ocaml/dune#1867, fix ocaml/dune#1862, @emillon) - Add the `(promote-into <dir>)` and `(promote-until-clean-into <dir>)` modes for `(rule ...)` stanzas, so that files can be promoted in another directory than the current one. For instance, this is used in merlin to promote menhir generated files in a directory that depends on the version of the compiler (ocaml/dune#1890, @diml) - Improve error message when `dune subst` fails (ocaml/dune#1898, fix ocaml/dune#1897, @rgrinberg) - Add more GC counters to catapult traces (fix908, @rgrinberg) - Add a preprocessor shim for the `let+` syntax of OCaml 4.08 (ocaml/dune#1899, implements ocaml/dune#1891, @diml) - Fix generation of `.merlin` files on Windows. `\` characters needed to be escaped (ocaml/dune#1869, @mlasson) - Fix 0 error code when `$ dune format-dune-file` fails. (ocaml/dune#1915, fix ocaml/dune#1914, @rgrinberg) - Configurator: deprecated `query_expr` and introduced `query_expr_err` which is the same but with a better error in case it fails. (ocaml/dune#1886, @ejgallego)

@aalekseyev

CHANGES: - Clean up watch mode polling loop: improves signal handling and error handling during polling (ocaml/dune#1912, fix ocaml/dune#1907, fix ocaml/dune#1671, @aalekseyev) - Change status messages during polling to be one-line, so that the messages are correctly erased by ^K. (ocaml/dune#1912, @aalekseyev) - Add support for `.cxx` extension for C++ stubs (ocaml/dune#1831, @rgrinberg) - Add `DUNE_WORKSPACE` variable. This variable is equivalent to setting `--workspace` in the command line. (ocaml/dune#1711, fix ocaml/dune#1503, @rgrinberg) - Add `c_flags` and `cxx_flags` to env profile settings (ocaml/dune#1700 and ocaml/dune#1800, @gretay-js) - Format `dune printenv` output (ocaml/dune#1867, fix ocaml/dune#1862, @emillon) - Add the `(promote-into <dir>)` and `(promote-until-clean-into <dir>)` modes for `(rule ...)` stanzas, so that files can be promoted in another directory than the current one. For instance, this is used in merlin to promote menhir generated files in a directory that depends on the version of the compiler (ocaml/dune#1890, @diml) - Improve error message when `dune subst` fails (ocaml/dune#1898, fix ocaml/dune#1897, @rgrinberg) - Add more GC counters to catapult traces (fix908, @rgrinberg) - Add a preprocessor shim for the `let+` syntax of OCaml 4.08 (ocaml/dune#1899, implements ocaml/dune#1891, @diml) - Fix generation of `.merlin` files on Windows. `\` characters needed to be escaped (ocaml/dune#1869, @mlasson) - Fix 0 error code when `$ dune format-dune-file` fails. (ocaml/dune#1915, fix ocaml/dune#1914, @rgrinberg) - Configurator: deprecated `query_expr` and introduced `query_expr_err` which is the same but with a better error in case it fails. (ocaml/dune#1886, @ejgallego) - Make sure `(menhir (mode promote) ...)` stanzas are ignored when using `--ignore-promoted-rules` or `-p` (ocaml/dune#1917, @diml)

ejgallego requested a review from rgrinberg as a code owner February 27, 2019 00:24

ejgallego commented Feb 27, 2019

View reviewed changes

ejgallego force-pushed the improve_query_err branch 6 times, most recently from 00a841d to dc75ff8 Compare February 27, 2019 01:02

ejgallego force-pushed the improve_query_err branch from dc75ff8 to 81ee509 Compare February 27, 2019 04:31

ejgallego force-pushed the improve_query_err branch from 81ee509 to fd62b13 Compare February 27, 2019 13:09

ejgallego force-pushed the improve_query_err branch from fd62b13 to 46cbc96 Compare February 27, 2019 17:04

ejgallego force-pushed the improve_query_err branch from 46cbc96 to 9b4ba9a Compare February 27, 2019 17:36

ejgallego force-pushed the improve_query_err branch from 9b4ba9a to 9860e70 Compare February 27, 2019 18:10

ejgallego force-pushed the improve_query_err branch from 9860e70 to a1efd95 Compare February 28, 2019 13:12

ejgallego force-pushed the improve_query_err branch from a1efd95 to fafd0b4 Compare February 28, 2019 13:19

rgrinberg approved these changes Feb 28, 2019

View reviewed changes

ejgallego force-pushed the improve_query_err branch from fafd0b4 to 73e496a Compare February 28, 2019 13:33

ejgallego requested a review from a user February 28, 2019 13:33

ejgallego requested a review from emillon as a code owner February 28, 2019 13:33

ejgallego force-pushed the improve_query_err branch 2 times, most recently from 9d4998f to b5ca0c0 Compare February 28, 2019 17:00

ejgallego requested a review from rgrinberg March 4, 2019 15:40

ejgallego force-pushed the improve_query_err branch from b5ca0c0 to 0020f90 Compare March 5, 2019 10:42

emillon approved these changes Mar 5, 2019

View reviewed changes

ejgallego mentioned this pull request Mar 5, 2019

[extension] Add (pkg_config ...) field. #1906

Open

rgrinberg closed this Mar 7, 2019

rgrinberg mentioned this pull request Mar 7, 2019

[new release] dune (1.8.0) ocaml/opam-repository#13603

Closed

rgrinberg mentioned this pull request Mar 7, 2019

[new release] dune (1.8.0) ocaml/opam-repository#13605

Merged

herbelin mentioned this pull request Apr 8, 2019

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

Closed

[configurator] Return pkg-config error to clients of new query API. #1886

[configurator] Return pkg-config error to clients of new query API. #1886

Conversation

ejgallego commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Feb 27, 2019

ejgallego commented Feb 27, 2019

ghost commented Feb 27, 2019

rgrinberg commented Feb 27, 2019

rgrinberg commented Feb 27, 2019

ejgallego commented Feb 27, 2019

ejgallego commented Feb 27, 2019

rgrinberg commented Feb 27, 2019

ejgallego commented Feb 27, 2019

ghost commented Feb 27, 2019

ejgallego commented Feb 27, 2019

rgrinberg commented Feb 28, 2019

rgrinberg commented Feb 28, 2019

rgrinberg commented Feb 28, 2019

ejgallego commented Feb 28, 2019 • edited Loading

ejgallego commented Feb 28, 2019

rgrinberg commented Feb 28, 2019

ejgallego commented Feb 28, 2019

rgrinberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejgallego commented Mar 4, 2019

rgrinberg commented Mar 4, 2019

emillon left a comment

Choose a reason for hiding this comment

ejgallego commented Mar 5, 2019

ghost commented Mar 5, 2019

ejgallego commented Mar 5, 2019

rgrinberg commented Mar 7, 2019

ejgallego commented Mar 7, 2019

rgrinberg commented Mar 7, 2019

ejgallego commented Feb 28, 2019 •

edited

Loading