Allow $PKG_CONFIG_PATH to have effect on macOS #1785

andreypopp · 2019-01-28T15:09:13Z

Previously brew's $PKG_CONFIG_PATH took precedence over $PKG_CONFIG_PATH, this commit changes so that already set $PKG_CONFIG_PATH have effect over brew's path.

We are seeing that with esy dune prefers globally installed libraries on macOS b/c of how dune overrides $PKG_CONFIG_PATH. This fixes that.

rgrinberg · 2019-01-28T17:32:38Z

Indeed the current behavior is bad, but I'm not sure if this PR fixes it. How would dune automatically use homebrew's sqlite for example? Perhaps that behavior is a bit too magical however. I think we'd be open to suggestions here.

andreypopp · 2019-01-29T07:49:12Z

Indeed the current behavior is bad, but I'm not sure if this PR fixes it. How would dune automatically use homebrew's sqlite for example?

The path for homebrew is still added, just that the previously set $PKG_CONFIG_PATH has precedence over it.

ghost · 2019-01-29T08:13:36Z

IIRC, I was the one who wrote it in this order originally as otherwise I couldn't get async_ssl to build on OSX. TBH, I still don't understand why we have to call brew manually and why brew just doesn't set this variable itself.

andreypopp · 2019-01-29T08:22:17Z

I just tried to build aantron's luv with libuv installed by homebrew and this patch applied and it worked (dune's log list homebrew location in -I flags).

I'm not sure why you weren't able to build async_ssl back then — my understanding is that even with this patch pkg-config should be able to find homebrew installed libs.

ghost · 2019-01-29T10:05:27Z

@andreypopp what happens if you remove the whole let env = match ocaml_config_var c "system" with | Some "macosx" -> begin ... in block?

Basically, I don't understand why we need this block at all. Surely it should be homebrew 's job to setup the environment. It makes no sense to me that dune has to be aware of homebrew.

andreypopp · 2019-01-29T11:32:16Z

@diml just tried it and it was able to find libuv installed with homebrew!

andreypopp · 2019-01-29T11:32:59Z

FWIW pkg-config libuv also works in terminal so indeed it seems setting PKG_CONFIG_PATH shouldn't be needed.

Should I remove it?

rgrinberg · 2019-01-29T12:00:55Z

I think the issue is that homebrew will refuse to make openssl available by default via pkg-config since OSX already has a systemwide SSL. I can confirm this on my laptop when listing all packages and not seeing $ pkg-config --list-all. Other brew packages are indeed listed here.

andreypopp · 2019-01-29T12:02:28Z

@rgrinberg yes, you need to link openssl keg manually with homebrew, this is what homebrew prints in info:

openssl is keg-only, which means it was not symlinked into /usr/local,
because Apple has deprecated use of OpenSSL in favor of its own TLS and crypto libraries.

If you need to have openssl first in your PATH run:
  echo 'export PATH="/usr/local/opt/openssl/bin:$PATH"' >> ~/.zshrc

For compilers to find openssl you may need to set:
  export LDFLAGS="-L/usr/local/opt/openssl/lib"
  export CPPFLAGS="-I/usr/local/opt/openssl/include"

For pkg-config to find openssl you may need to set:
  export PKG_CONFIG_PATH="/usr/local/opt/openssl/lib/pkgconfig"

rgrinberg · 2019-01-29T12:04:26Z

So I suppose this hack should go into the various ssl packages.

ghost · 2019-01-29T12:24:10Z

Agreed. What about keeping the code in configurator but making it an explicit opt-in? For instance we could add a ?force_homebrew_on_osx:bool flag to Pkg_config.query.

rgrinberg · 2019-01-29T12:58:45Z

That would work but it's probably still too forceful in general. We don't want to force homebrew versions on users even when they have it installed. I don't have any better suggestions however, so I'd accept such a change.

andreypopp · 2019-01-29T13:02:49Z

my 2cents: I'd remove homebrew paths completely and ask users to follow homebrew's instructions for any specific package (if any).

ghost · 2019-01-29T13:08:33Z

That's what I would do as well, but I remember that it wasn't very easy for users. /cc @avsm, I remember that we discussed this in the past.

avsm · 2019-01-29T14:19:39Z

I think the problem is how to lift this choice (Homebrew SSL or systemwide one) into the dune build invocation. Isn't this tied to #1579 so that the choice can be moved into configurator but still manipulated by a user?

Either way that's a configurator problem. Not setting PKG_CONFIG_PATH in Dune sounds saner to me, and at a minimum this PR still does the right thing by using the local dune versions first.

andreypopp · 2019-01-29T16:26:05Z

Let me know what's the best course of action then! I think the fix in this PR is relatively safe.

Another possible solution is to make conf-openssl package query brew for installed openssl package and set $PKG_CONFIG_PATH instead of dune doing that.

rgrinberg · 2019-01-30T03:47:37Z

I think we can just merge this PR and apply the appropriate fix to conf-ssl. Unless anyone objects, I will merge this PR once you add a CHANGES entry

Previously brew's $PKG_CONFIG_PATH took precedence over $PKG_CONFIG_PATH, this commit changes so that already set $PKG_CONFIG_PATH have effect over brew's path. Signed-off-by: Andrey Popp <8mayday@gmail.com>

Signed-off-by: Andrey Popp <8mayday@gmail.com>

andreypopp · 2019-01-30T07:14:52Z

I think we can just merge this PR and apply the appropriate fix to conf-ssl. Unless anyone objects, I will merge this PR once you add a CHANGES entry

Done. I still don't think conf-ssl should be updated as brew's path will still end in $PKG_CONFIG_PATH just appended and not prepended.

ghost · 2019-01-30T08:34:04Z

Now that I understand things a bit better, I realise that this PR in its current form is likely to break the build of async_ssl on OSX. Do you agree?

If async_ssl is the only affected package, then what we can do is release a new version of async_ssl with the brew hack directly in async_ssl, then just drop the call to brew altogether in dune and add a version constraint on dune in async_ssl.

Allowing the user to control which version is selected would be nice, however it seems to me that this is an improvement over the current situation, so there is no urgency on this side.

BTW, I don't know what conf-ssl does or where it is used, so I don't know how it is affected by all this.

andreypopp · 2019-01-30T12:55:13Z

Now that I understand things a bit better, I realise that this PR in its current form is likely to break the build of async_ssl on OSX. Do you agree?

I disagree, this PR shouldn't break async_ssl (or any other sensible
build configuration), why do you think it should break it?

So I've tried to build async_ssl (master) with this PR and it's failed for me.
But it also failed for me without this PR with dune released on opam. The error
message is the same in both configurations:

openssl_helpers.c:2:10: fatal error: 'openssl/x509v3.h' file not found
#include <openssl/x509v3.h>
         ^~~~~~~~~~~~~~~~~~
1 error generated.

And apparently the bindings/dune isn't passing ccopt flags discovered by
configurator. If I fix it with this patch:

diff --git a/bindings/dune b/bindings/dune
index 93126a3..50e42a7 100644
--- a/bindings/dune
+++ b/bindings/dune
@@ -12,8 +12,8 @@

 (library (name async_ssl_bindings) (public_name async_ssl.bindings)
  (c_names openssl_helpers)
- (c_flags (:standard \ -Werror -pedantic -Wall -Wunused))
+ (c_flags (:standard (:include openssl-ccopt) \ -Werror -pedantic -Wall -Wunused))
  (c_library_flags :standard (:include openssl-cclib.sexp))
  (libraries ctypes.stubs ctypes ctypes.foreign.threaded)
  (virtual_deps conf-openssl) (preprocessor_deps config.h)

Then it works well and builds both with stock dune and with dune patched with
this PR.

I also inspected openssl-{cclib,ccopt}.sexp and they have strings pointing to
brew installed version of openssl.

Allowing the user to control which version is selected would be nice, however it seems to me that this is an improvement over the current situation, so there is no urgency on this side.

I disagree as this makes esy builds non deterministic - users might have
incorrect versions of software installed via brew and it might fail as esy
packages might expect newer versions. Moreover there's no workaround but to use
the patched dune version.

ghost · 2019-01-30T13:48:17Z

Thanks for testing with master, it seems that this problem was introduced since the last release in opam. /cc @xclerc we need to add the (:include openssl-ccopt) before the next release.

Then it works well and builds both with stock dune and with dune patched with this PR.

I also inspected openssl-{cclib,ccopt}.sexp and they have strings pointing to brew installed version of openssl.

I'm not really sure what's going on. Could you try removing the call to brew entirely and see if you get the same behaviour? If yes, then it's likely we just don't need it anymore.

I disagree as this makes esy builds non deterministic - users might have
incorrect versions of software installed via brew and it might fail as esy
packages might expect newer versions. Moreover there's no workaround but to use
the patched dune version.

I'm not sure we were talking about the same thing. @avsm was basically suggesting an option such as dune configure --use-brew-ssl or dune configure --use-system-wide-ssl to force the use of one or the other version. I don't believe this introduce non determinism.

andreypopp · 2019-01-30T14:01:48Z

Then it works well and builds both with stock dune and with dune patched with this PR.

I also inspected openssl-{cclib,ccopt}.sexp and they have strings pointing to brew installed version of openssl.

I'm not really sure what's going on. Could you try removing the call to brew entirely and see if you get the same behaviour? If yes, then it's likely we just don't need it anymore.

The reason it still finds the brew's openssl is that this PR only changes the order of $PKG_CONFIG_PATH — it appends rather than prepends brew's path.

I've tried to remove brew's paths completely and it fails now:

discover.c:2:10: fatal error: 'openssl/ssl.h' file not found
#include <openssl/ssl.h>
         ^~~~~~~~~~~~~~~
1 error generated.

and openssl-{cclib,ccopt}.sexp contain empty lists () as expected.

andreypopp · 2019-01-30T14:04:11Z

I'm not sure we were talking about the same thing. @avsm was basically suggesting an option such as dune configure --use-brew-ssl or dune configure --use-system-wide-ssl to force the use of one or the other version. I don't believe this introduce non determinism.

Sorry, I was indeed confused. I thought you are referring to the entire PR.

ghost · 2019-01-30T14:56:28Z

I see. I thought PKG_CONFIG_PATH would be set to point to the system directories by default, and so changing the order would have an effect on which version of openssl is selected.

I have no objection on the current PR then.

andreypopp · 2019-01-30T15:00:30Z

I see. I thought PKG_CONFIG_PATH would be set to point to the system directories by default, and so changing the order would have an effect on which version of openssl is selected.

By default you usually don't have $PKG_CONFIG_PATH set I believe. But if you have — then this PR will change the behaviour of dune — but in a sensible way I think — global brew's installations will still be available but as a fallabck after custom set $PKG_CONFIG_PATH is inspected by pkg-config.

ghost · 2019-01-30T15:06:58Z

That makes sense to me

vsiles · 2019-01-31T11:11:38Z

For the record, I'm trying to use Configurator.V1.Pkg_config on a linux and a mac, and in both cases my PKG_CONFIG_PATH variable is totally ignored.

andreypopp · 2019-01-31T11:36:41Z

@vsiles it depends on if you are using opam or esy. Ping me on discord and I can help with either.

vsiles · 2019-01-31T12:07:29Z

@vsiles it depends on if you are using opam or esy. Ping me on discord and I can help with either.

I'm very new to discord, how do I find you ?

andreypopp · 2019-01-31T14:03:29Z

@vsiles my nick is andreypopp there, I'm also on IRC freenode under the same nickname.

andreypopp · 2019-02-04T16:32:23Z

Friendly ping here.

rgrinberg · 2019-02-04T17:51:42Z

I merged manually

@diml

CHANGES: - Second step of the deprecation of jbuilder: the `jbuilder` binary now emits a warning on every startup and both `jbuilder` and `dune` emit warnings when encountering `jbuild` files (ocaml/dune#1752, @diml) - Change the layout of build artifacts inside _build. The new layout enables optimizations that depend on the presence of `.cmx` files of private modules (ocaml/dune#1676, @bobot) - Fix merlin handling of private module visibility (ocaml/dune#1653 @bobot) - unstable-fmt: use boxes to wrap some lists (ocaml/dune#1608, fix ocaml/dune#1153, @emillon, thanks to @rgrinberg) - skip directories when looking up programs in the PATH (ocaml/dune#1628, fixes ocaml/dune#1616, @diml) - Use `lsof` on macOS to implement `--stats` (ocaml/dune#1636, fixes ocaml/dune#1634, @xclerc) - Generate `dune-package` files for every package. These files are installed and read instead of `META` files whenever they are available (ocaml/dune#1329, @rgrinberg) - Fix preprocessing for libraries with `(include_subdirs ..)` (ocaml/dune#1624, fix ocaml/dune#1626, @nojb, @rgrinberg) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - When executing actions, open files lazily and close them as soon as possible in order to reduce the maximum number of file descriptors opened by Dune (ocaml/dune#1635, ocaml/dune#1643, fixes ocaml/dune#1633, @jonludlam, @rgrinberg, @diml) - Reimplement the core of Dune using a new generic memoization system (ocaml/dune#1489, @rudihorn, @diml) - Replace the broken cycle detection algorithm by a state of the art one from [this paper](https://doi.org/10.1145/2756553) (ocaml/dune#1489, @rudihorn) - Get the correct environment node for multi project workspaces (ocaml/dune#1648, @rgrinberg) - Add `dune compute` to call internal memoized functions (ocaml/dune#1528, @rudihorn, @diml) - Add `--trace-file` option to trace dune internals (ocaml/dune#1639, fix ocaml/dune#1180, @emillon) - Add `--no-print-directory` (borrowed from GNU make) to suppress `Entering directory` messages. (ocaml/dune#1668, @dra27) - Remove `--stats` and track fd usage in `--trace-file` (ocaml/dune#1667, @emillon) - Add virtual libraries feature and enable it by default (ocaml/dune#1430 fixes ocaml/dune#921, @rgrinberg) - Fix handling of Control+C in watch mode (ocaml/dune#1678, fixes ocaml/dune#1671, @diml) - Look for jsoo runtime in the same dir as the `js_of_ocaml` binary when the ocamlfind package is not available (ocaml/dune#1467, @nojb) - Make the `seq` package available for OCaml >= 4.07 (ocaml/dune#1714, @rgrinberg) - Add locations to error messages where a rule fails to generate targets and rules that require files outside the build/source directory. (ocaml/dune#1708, fixes ocaml/dune#848, @rgrinberg) - Let `Configurator` handle `sizeof` (in addition to negative numbers). (ocaml/dune#1726, fixes ocaml/dune#1723, @Chris00) - Fix an issue causing menhir generated parsers to fail to build in some cases. The fix is to systematically use `-short-paths` when calling `ocamlc -i` (ocaml/dune#1743, fix ocaml/dune#1504, @diml) - Never raise when printing located errors. The code that would print the location excerpts was prone to raising. (ocaml/dune#1744, fix ocaml/dune#1736, @rgrinberg) - Add a `dune upgrade` command for upgrading jbuilder projects to Dune (ocaml/dune#1749, @diml) - When automatically creating a `dune-project` file, insert the detected name in it (ocaml/dune#1749, @diml) - Add `(implicit_transitive_deps <bool>)` mode to dune projects. When this mode is turned off, transitive dependencies are not accessible. Only listed dependencies are directly accessible. (ocaml/dune#1734, ocaml/dune#430, @rgrinberg, @hnrgrgr) - Add `toplevel` stanza. This stanza is used to define toplevels with libraries already preloaded. (ocaml/dune#1713, @rgrinberg) - Generate `.merlin` files that account for normal preprocessors defined using a subset of the `action` language. (ocaml/dune#1768, @rgrinberg) - Emit `(orig_src_dir <path>)` metadata in `dune-package` for dune packages built with `--store-orig-source-dir` command line flag (also controlled by `DUNE_STORE_ORIG_SOURCE_DIR` env variable). This is later used to generate `.merlin` with `S`-directives pointed to original source locations and thus allowing merlin to see those. (ocaml/dune#1750, @andreypopp) - Improve the behavior of `dune promote` when the files to be promoted have been deleted. (ocaml/dune#1775, fixes ocaml/dune#1772, @diml) - unstable-fmt: preserve comments (ocaml/dune#1766, @emillon) - Pass flags correctly when using `staged_pps` (ocaml/dune#1779, fixes ocaml/dune#1774, @diml) - Fix an issue with the use of `(mode promote)` in the menhir stanza. It was previously causing intermediate *mock* files to be promoted (ocaml/dune#1783, fixes ocaml/dune#1781, @diml) - unstable-fmt: ignore files using OCaml syntax (ocaml/dune#1784, @emillon) - Configurator: Add `which` function to replace the `which` command line utility in a cross platform way. (ocaml/dune#1773, fixes ocaml/dune#1705, @Chris00) - Make configurator append paths to `$PKG_CONFIG_PATH` on macOS. Previously it was prepending paths and thus `$PKG_CONFIG_PATH` set by users could have been overridden by homebrew installed libraries (ocaml/dune#1785, @andreypopp) - Disallow c/cxx sources that share an object file in the same stubs archive. This means that `foo.c` and `foo.cpp` can no longer exist in the same library. (ocaml/dune#1788, @rgrinberg) - Forbid use of `%{targets}` (or `${@}` in jbuild files) inside preprocessing actions (ocaml/dune#1812, fixes ocaml/dune#1811, @diml) - Add `DUNE_PROFILE` environment variable to easily set the profile. (ocaml/dune#1806, @rgrinberg) - Deprecate the undocumented `(no_keep_locs)` field. It was only necessary until virtual libraries were supported (ocaml/dune#1822, fix ocaml/dune#1816, @diml) - Rename `unstable-fmt` to `format-dune-file` and remove its `--inplace` option. (ocaml/dune#1821, @emillon). - Autoformatting: `(using fmt 1.1)` will also format dune files (ocaml/dune#1821, @emillon). - Autoformatting: record dependencies on `.ocamlformat-ignore` files (ocaml/dune#1824, fixes ocaml/dune#1793, @emillon)

andreypopp requested a review from rgrinberg as a code owner January 28, 2019 15:09

andreypopp force-pushed the andreypopp/pkg-config-order branch from 56e5793 to b486b46 Compare January 28, 2019 15:10

andreypopp added 2 commits January 30, 2019 10:13

Allow $PKG_CONFIG_PATH to have effect on macOS

8794cd4

Previously brew's $PKG_CONFIG_PATH took precedence over $PKG_CONFIG_PATH, this commit changes so that already set $PKG_CONFIG_PATH have effect over brew's path. Signed-off-by: Andrey Popp <8mayday@gmail.com>

Update CHANGES

18e6abc

Signed-off-by: Andrey Popp <8mayday@gmail.com>

andreypopp force-pushed the andreypopp/pkg-config-order branch from 06e22fe to 18e6abc Compare January 30, 2019 07:13

ghost mentioned this pull request Jan 31, 2019

Configurator.V1 discards PKG_CONFIG_PATH on most platform #1790

Closed

rgrinberg closed this Feb 4, 2019

rgrinberg mentioned this pull request Feb 12, 2019

[new release] dune (1.7.0) ocaml/opam-repository#13444

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow $PKG_CONFIG_PATH to have effect on macOS #1785

Allow $PKG_CONFIG_PATH to have effect on macOS #1785

andreypopp commented Jan 28, 2019

rgrinberg commented Jan 28, 2019

andreypopp commented Jan 29, 2019 •

edited

Loading

ghost commented Jan 29, 2019

andreypopp commented Jan 29, 2019

ghost commented Jan 29, 2019 •

edited by ghost

Loading

andreypopp commented Jan 29, 2019

andreypopp commented Jan 29, 2019

rgrinberg commented Jan 29, 2019

andreypopp commented Jan 29, 2019

rgrinberg commented Jan 29, 2019 •

edited

Loading

ghost commented Jan 29, 2019

rgrinberg commented Jan 29, 2019

andreypopp commented Jan 29, 2019

ghost commented Jan 29, 2019

avsm commented Jan 29, 2019

andreypopp commented Jan 29, 2019

rgrinberg commented Jan 30, 2019

andreypopp commented Jan 30, 2019 •

edited

Loading

ghost commented Jan 30, 2019

andreypopp commented Jan 30, 2019

ghost commented Jan 30, 2019

andreypopp commented Jan 30, 2019

andreypopp commented Jan 30, 2019

ghost commented Jan 30, 2019

andreypopp commented Jan 30, 2019

ghost commented Jan 30, 2019

vsiles commented Jan 31, 2019

andreypopp commented Jan 31, 2019

vsiles commented Jan 31, 2019

andreypopp commented Jan 31, 2019

andreypopp commented Feb 4, 2019

rgrinberg commented Feb 4, 2019

Allow $PKG_CONFIG_PATH to have effect on macOS #1785

Allow $PKG_CONFIG_PATH to have effect on macOS #1785

Conversation

andreypopp commented Jan 28, 2019

rgrinberg commented Jan 28, 2019

andreypopp commented Jan 29, 2019 • edited Loading

ghost commented Jan 29, 2019

andreypopp commented Jan 29, 2019

ghost commented Jan 29, 2019 • edited by ghost Loading

andreypopp commented Jan 29, 2019

andreypopp commented Jan 29, 2019

rgrinberg commented Jan 29, 2019

andreypopp commented Jan 29, 2019

rgrinberg commented Jan 29, 2019 • edited Loading

ghost commented Jan 29, 2019

rgrinberg commented Jan 29, 2019

andreypopp commented Jan 29, 2019

ghost commented Jan 29, 2019

avsm commented Jan 29, 2019

andreypopp commented Jan 29, 2019

rgrinberg commented Jan 30, 2019

andreypopp commented Jan 30, 2019 • edited Loading

ghost commented Jan 30, 2019

andreypopp commented Jan 30, 2019

ghost commented Jan 30, 2019

andreypopp commented Jan 30, 2019

andreypopp commented Jan 30, 2019

ghost commented Jan 30, 2019

andreypopp commented Jan 30, 2019

ghost commented Jan 30, 2019

vsiles commented Jan 31, 2019

andreypopp commented Jan 31, 2019

vsiles commented Jan 31, 2019

andreypopp commented Jan 31, 2019

andreypopp commented Feb 4, 2019

rgrinberg commented Feb 4, 2019

andreypopp commented Jan 29, 2019 •

edited

Loading

ghost commented Jan 29, 2019 •

edited by ghost

Loading

rgrinberg commented Jan 29, 2019 •

edited

Loading

andreypopp commented Jan 30, 2019 •

edited

Loading