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

fix: preserve directory hierarchy for installed headers #7512

Merged
merged 1 commit into from
Apr 13, 2023
Merged

fix: preserve directory hierarchy for installed headers #7512

merged 1 commit into from
Apr 13, 2023

Conversation

rgrinberg
Copy link
Member

If we have foo/bar.h, we'll preserve the hierarchy relative to the
library directory

Signed-off-by: Rudi Grinberg me@rgrinberg.com

@rgrinberg rgrinberg requested a review from anmonteiro April 7, 2023 02:48
@rgrinberg rgrinberg force-pushed the ps/rr/fix__preserve_directory_hierarchy_for_installed_headers branch from 1b630b2 to b147fd6 Compare April 7, 2023 03:11
@rgrinberg rgrinberg added this to the 3.8.0 milestone Apr 7, 2023
@rgrinberg rgrinberg force-pushed the ps/rr/fix__preserve_directory_hierarchy_for_installed_headers branch 3 times, most recently from e40419e to bf031e1 Compare April 7, 2023 06:38
@rgrinberg
Copy link
Member Author

rgrinberg commented Apr 7, 2023 •
edited
Loading

Let's have a look from @emillon as well

@rgrinberg rgrinberg requested a review from emillon April 7, 2023 16:07
@rgrinberg rgrinberg mentioned this pull request Apr 7, 2023
Closed
@rgrinberg
Copy link
Member Author

On second thought, this fix is a little bit weird. For example, how would we handle something like (install_c_headers ../foo)?

Probably simpler to introduce the syntax from the install stanza and allow users to select the installed path. E.g. (install_c_headers (./foo as baz)).

To make the common case simple, we could introduce a new constructor such as (install_c_headers (with_subdir foo/bar)) that will have the same behavior as this PR.

@rgrinberg rgrinberg force-pushed the ps/rr/fix__preserve_directory_hierarchy_for_installed_headers branch from bf031e1 to 5b7c145 Compare April 8, 2023 00:47
@rgrinberg
Copy link
Member Author

Okay, I just switched to using normal (foo as bar) patterns.

It requires manual work to get the install layout we want, but at least it's obvious and consistent with the rest of dune.

@rgrinberg
Copy link
Member Author

Actually, I'm now leaning towards introducing a new field now. One thing I dislike about this implementation is that it makes globbing awkard. E.g. one cannot do (install_c_headers (glob_files *.h)) for seemingly no good reason.

The new field is just going to be public_headers and it will allow proper filenames. As a bonus, this will allow header extensions such as .hh and .hpp that should be usable as well.

@rgrinberg rgrinberg force-pushed the ps/rr/fix__preserve_directory_hierarchy_for_installed_headers branch from 5b7c145 to 666553c Compare April 8, 2023 16:31
@rgrinberg
Copy link
Member Author

@anmonteiro take another look please

@rgrinberg
Copy link
Member Author

@anmonteiro I rewrote this feature again. This time with the realization that this almost exactly the same as melange's runtime deps feature.

@rgrinberg rgrinberg force-pushed the ps/rr/fix__preserve_directory_hierarchy_for_installed_headers branch from 666553c to a9f317c Compare April 10, 2023 01:15
anmonteiro
anmonteiro approved these changes Apr 10, 2023
View reviewed changes
Copy link
Collaborator

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. It looks very similar to runtime_deps indede.

src/dune_rules/install_rules.ml Outdated Show resolved Hide resolved
@rgrinberg rgrinberg force-pushed the ps/rr/fix__preserve_directory_hierarchy_for_installed_headers branch from a9f317c to 987647e Compare April 10, 2023 02:54
emillon
emillon approved these changes Apr 11, 2023
View reviewed changes
Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That's pretty much what I had in mind in an early implementation.

src/dune_rules/lib_flags.ml Outdated Show resolved Hide resolved
src/dune_rules/lib_flags.ml Outdated Show resolved Hide resolved
src/dune_rules/dune_file.ml Show resolved Hide resolved
@rgrinberg rgrinberg linked an issue Apr 12, 2023 that may be closed by this pull request
-I option not passed during development for headers installed by a sibling package from a monorepo #4121
Closed
@rgrinberg
feature: introduce public_headers
5fc72f2 
This field works like install_c_headers except it allows full file names
and allows to customize the destination paths

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: cf542cda-1684-4719-bffd-fdb6a8bc2d63 -->
@rgrinberg rgrinberg force-pushed the ps/rr/fix__preserve_directory_hierarchy_for_installed_headers branch from 987647e to 5fc72f2 Compare April 13, 2023 22:01
@rgrinberg
Copy link
Member Author

@anmonteiro i've now forbidden all non file deps in public_headers. A similar change to melange runtime deps could be done.

@rgrinberg rgrinberg merged commit 1c62a2d into main Apr 13, 2023
@rgrinberg rgrinberg deleted the ps/rr/fix__preserve_directory_hierarchy_for_installed_headers branch April 13, 2023 22:11
@anmonteiro
Copy link
Collaborator

Thanks for the heads up, I’ll apply the fix there too.

@aantron aantron mentioned this pull request Apr 14, 2023
Closed
@emillon emillon mentioned this pull request Apr 18, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Apr 18, 2023
@emillon
[new release] dune (15 packages) (3.8.0~alpha1)
8f04d31 
CHANGES:

- When a rule's action is interrupted, delete any leftover directory targets.
  This is consistent with how we treat file targets. (@rgrinberg, 7564)

- Fix plugin loading with findlib. The functionality was broken in 3.7.0.
  (ocaml/dune#7556, @anmonteiro)

- Introduce a `public_headers` field on libraries. This field is like
  `install_c_headers`, but it allows to choose the extension and choose the
  paths for the installed headers. (ocaml/dune#7512, @rgrinberg)

- Load the host context `findlib.conf` when cross-compiling (ocaml/dune#7428, fixes
  ocaml/dune#1701, @rgrinberg, @anmonteiro)

- Resolve `ppx_runtime_libraries` in the target context when cross compiling
  (ocaml/dune#7450, fixes ocaml/dune#2794, @anmonteiro)

- Use `$PKG_CONFIG`, when set, to find the `pkg-config` binary  (ocaml/dune#7469, fixes
  ocaml/dune#2572, @anmonteiro)

- Preliminary support for Coq compiled intefaces (`.vos` files) enabled via
  `(mode vos)` in `coq.theory` stanzas. This can be used in combination with
  `dune coq top` to obtain fast re-building of dependencies (with no checking
  of proofs) prior to stepping into a file. (ocaml/dune#7406, @rlepigre)

- Fix dune crashing on MacOS in watch mode whenever `$PATH` contains `$PWD`
  (ocaml/dune#7441, fixes ocaml/dune#6907, @rgrinberg)

- Fix `dune install` when cross compiling (ocaml/dune#7410, fixes ocaml/dune#6191, @anmonteiro,
  @rizo)

- Find `pps` dependencies in the host context when cross-compiling,  (ocaml/dune#7410,
  fixes ocaml/dune#4156, @anmonteiro)

- Dune in watch mode no longer builds concurrent rules in serial (ocaml/dune#7395
  @rgrinberg, @jchavarri)

- `dune coq top` now correctly respects the project root when called from a
  subdirectory. However, absolute filenames passed to `dune coq top` are no
  longer supported (due to being buggy) (ocaml/dune#7357, fixes ocaml/dune#7344, @rlepigre and
  @Alizter)

- Added a `--no-build` option to `dune coq top` for avoiding rebuilds (ocaml/dune#7380,
  fixes ocaml/dune#7355, @Alizter)

- RPC: Ignore SIGPIPE when clients suddenly disconnect (ocaml/dune#7299, ocaml/dune#7319, fixes
  ocaml/dune#6879, @rgrinberg)

- Always clean up the UI on exit. (ocaml/dune#7271, fixes ocaml/dune#7142 @rgrinberg)

- Bootstrap: remove reliance on shell. Previously, we'd use the shell to get
  the number of processors. (ocaml/dune#7274, @rgrinberg)

- Bootstrap: correctly detect the number of processors by allowing `nproc` to be
  looked up in `$PATH` (ocaml/dune#7272, @Alizter)

- Speed up file copying on macos by using `clonefile` when available
  (@rgrinberg, ocaml/dune#7210)

- Adds support for loading plugins in toplevels (ocaml/dune#6082, fixes ocaml/dune#6081,
  @ivg, @richardlford)

- Support commands that output 8-bit and 24-bit colors in the terminal (ocaml/dune#7188,
  @Alizter)

- Speed up rule generation for libraries and executables with many modules
  (ocaml/dune#7187, @jchavarri)

- Add `--watch-exclusions` to Dune build options (ocaml/dune#7216, @jonahbeckford)

- Do not re-render UI on every frame if the UI doesn't change (ocaml/dune#7186, fix
  ocaml/dune#7184, @rgrinberg)

- Make coq_db creation in scope lazy (@ejgallego, ocaml/dune#7133)

- Non-user proccesses such as version control or config checking are now run
  silently. (ocaml/dune#6994, fixes ocaml/dune#4066, @Alizter)

- Add the `--display-separate-messages` flag to separate the error messages
  produced by commands with a blank line. (ocaml/dune#6823, fixes ocaml/dune#6158, @esope)

- Accept the Ordered Set Language for the `modes` field in `library` stanzas
  (ocaml/dune#6611, @anmonteiro).

- dune install now respects --display quiet mode (ocaml/dune#7116, fixes ocaml/dune#4573, fixes
  ocaml/dune#7106, @Alizter)

- Stub shared libraries (dllXXX_stubs.so) in Dune-installed libraries could not
  be used as dependencies of libraries in the workspace (eg when compiling to
  bytecode and/or Javascript).  This is now fixed. (ocaml/dune#7151, @nojb)

- Allow the main module of a library with `(stdlib ...)` to depend on other
  libraries (ocaml/dune#7154, @anmonteiro).

- Bytecode executables built for JSOO are linked with `-noautolink` and no
  longer depend on the shared stubs of their dependent libraries (ocaml/dune#7156, @nojb)

- Added a new user action `(concurrent )` which is like `(progn )` but runs the
  actions concurrently. (ocaml/dune#6933, @Alizter)

- Allow `(stdlib ...)` to be used with `(wrapped false)` in library stanzas
  (ocaml/dune#7139, @anmonteiro).

- Allow parallel execution of inline tests partitions (ocaml/dune#7012, @hhugo)

- Support `(link_flags ...)` in `(cinaps ...)` stanza. (ocaml/dune#7423, fixes ocaml/dune#7416,
  @nojb)

- Allow `(package ...)` in any position within `(rule ...)` stanza (ocaml/dune#7445,
  @Leonidas-from-XIV)

- Always include `opam` files in the generated `.install` file. Previously, it
  would not be included whenever `(generate_opam_files true)` was set and the
  `.install` file wasn't yet generated. (ocaml/dune#7547, @rgrinberg)
@emillon emillon mentioned this pull request May 23, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request May 23, 2023
@emillon
[new release] dune (15 packages) (3.8.0)
df1ab22 
CHANGES:

- Fix string quoting in the json file written by `--trace-file` (ocaml/dune#7773,
  @rleshchinskiy)

- Read `pkg-config` arguments from the `PKG_CONFIG_ARGN` environment variable
  (ocaml/dune#1492, ocaml/dune#7734, @anmonteiro)

- Correctly set `MANPATH` in `dune exec`. Previously, we would use the `bin/`
  directory of the context. (ocaml/dune#7655, @rgrinberg)

- Allow overriding the `ocaml` binary with findlib configuration (ocaml/dune#7648,
  @rgrinberg)

- merlin: ignore instrumentation settings for preprocessing. (ocaml/dune#7606, fixes
  ocaml/dune#7465, @Alizter)

- When a rule's action is interrupted, delete any leftover directory targets.
  This is consistent with how we treat file targets. (ocaml/dune#7564, @rgrinberg)

- Fix plugin loading with findlib. The functionality was broken in 3.7.0.
  (ocaml/dune#7556, @anmonteiro)

- Introduce a `public_headers` field on libraries. This field is like
  `install_c_headers`, but it allows to choose the extension and choose the
  paths for the installed headers. (ocaml/dune#7512, @rgrinberg)

- Load the host context `findlib.conf` when cross-compiling (ocaml/dune#7428, fixes
  ocaml/dune#1701, @rgrinberg, @anmonteiro)

- Add a `coqdoc_flags` field to the `coq.theory` stanza allowing the user to
  pass extra arguments to `coqdoc`. (ocaml/dune#7676, fixes ocaml/dune#7954 @Alizter)

- Resolve `ppx_runtime_libraries` in the target context when cross compiling
  (ocaml/dune#7450, fixes ocaml/dune#2794, @anmonteiro)

- Use `$PKG_CONFIG`, when set, to find the `pkg-config` binary  (ocaml/dune#7469, fixes
  ocaml/dune#2572, @anmonteiro)

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will cause Dune to emit a warning which will become an error in 3.9. (ocaml/dune#7608,
  fixes ocaml/dune#7026, @Alizter)

- Preliminary support for Coq compiled intefaces (`.vos` files) enabled via
  `(mode vos)` in `coq.theory` stanzas. This can be used in combination with
  `dune coq top` to obtain fast re-building of dependencies (with no checking
  of proofs) prior to stepping into a file. (ocaml/dune#7406, @rlepigre)

- Fix dune crashing on MacOS in watch mode whenever `$PATH` contains `$PWD`
  (ocaml/dune#7441, fixes ocaml/dune#6907, @rgrinberg)

- Fix `dune install` when cross compiling (ocaml/dune#7410, fixes ocaml/dune#6191, @anmonteiro,
  @rizo)

- Find `pps` dependencies in the host context when cross-compiling,  (ocaml/dune#7410,
  fixes ocaml/dune#4156, @anmonteiro)

- Dune in watch mode no longer builds concurrent rules in serial (ocaml/dune#7395
  @rgrinberg, @jchavarri)

- Dune can now detect Coq theories from outside the workspace. This allows for
  composition with installed theories (not necessarily installed with Dune).
  (ocaml/dune#7047, @Alizter, @ejgallego)

- `dune coq top` now correctly respects the project root when called from a
  subdirectory. However, absolute filenames passed to `dune coq top` are no
  longer supported (due to being buggy) (ocaml/dune#7357, fixes ocaml/dune#7344, @rlepigre and
  @Alizter)

- Added a `--no-build` option to `dune coq top` for avoiding rebuilds (ocaml/dune#7380,
  fixes ocaml/dune#7355, @Alizter)

- RPC: Ignore SIGPIPE when clients suddenly disconnect (ocaml/dune#7299, ocaml/dune#7319, fixes
  ocaml/dune#6879, @rgrinberg)

- Always clean up the UI on exit. (ocaml/dune#7271, fixes ocaml/dune#7142 @rgrinberg)

- Bootstrap: remove reliance on shell. Previously, we'd use the shell to get
  the number of processors. (ocaml/dune#7274, @rgrinberg)

- Bootstrap: correctly detect the number of processors by allowing `nproc` to be
  looked up in `$PATH` (ocaml/dune#7272, @Alizter)

- Speed up file copying on macos by using `clonefile` when available
  (@rgrinberg, ocaml/dune#7210)

- Adds support for loading plugins in toplevels (ocaml/dune#6082, fixes ocaml/dune#6081,
  @ivg, @richardlford)

- Support commands that output 8-bit and 24-bit colors in the terminal (ocaml/dune#7188,
  @Alizter)

- Speed up rule generation for libraries and executables with many modules
  (ocaml/dune#7187, @jchavarri)

- Add `--watch-exclusions` to Dune build options (ocaml/dune#7216, @jonahbeckford)

- Do not re-render UI on every frame if the UI doesn't change (ocaml/dune#7186, fix
  ocaml/dune#7184, @rgrinberg)

- Make coq_db creation in scope lazy (@ejgallego, ocaml/dune#7133)

- Non-user proccesses such as version control or config checking are now run
  silently. (ocaml/dune#6994, fixes ocaml/dune#4066, @Alizter)

- Add the `--display-separate-messages` flag to separate the error messages
  produced by commands with a blank line. (ocaml/dune#6823, fixes ocaml/dune#6158, @esope)

- Accept the Ordered Set Language for the `modes` field in `library` stanzas
  (ocaml/dune#6611, @anmonteiro).

- dune install now respects --display quiet mode (ocaml/dune#7116, fixes ocaml/dune#4573, fixes
  ocaml/dune#7106, @Alizter)

- Stub shared libraries (dllXXX_stubs.so) in Dune-installed libraries could not
  be used as dependencies of libraries in the workspace (eg when compiling to
  bytecode and/or Javascript).  This is now fixed. (ocaml/dune#7151, @nojb)

- Allow the main module of a library with `(stdlib ...)` to depend on other
  libraries (ocaml/dune#7154, @anmonteiro).

- Bytecode executables built for JSOO are linked with `-noautolink` and no
  longer depend on the shared stubs of their dependent libraries (ocaml/dune#7156, @nojb)

- Added a new user action `(concurrent )` which is like `(progn )` but runs the
  actions concurrently. (ocaml/dune#6933, @Alizter)

- Allow `(stdlib ...)` to be used with `(wrapped false)` in library stanzas
  (ocaml/dune#7139, @anmonteiro).

- Allow parallel execution of inline tests partitions (ocaml/dune#7012, @hhugo)

- Support `(link_flags ...)` in `(cinaps ...)` stanza. (ocaml/dune#7423, fixes ocaml/dune#7416,
  @nojb)

- Allow `(package ...)` in any position within `(rule ...)` stanza (ocaml/dune#7445,
  @Leonidas-from-XIV)

- Always include `opam` files in the generated `.install` file. Previously, it
  would not be included whenever `(generate_opam_files true)` was set and the
  `.install` file wasn't yet generated. (ocaml/dune#7547, @rgrinberg)

- Fix regression where Merlin was unable to handle filenames with uppercase
  letters under Windows. (ocaml/dune#7577, @nojb)

- On nix+macos, pass `-f` to the codesign hook to avoid errors when the binary
  is already signed (ocaml/dune#7183, fixes ocaml/dune#6265, @greedy)

- Fix bug where RPC clients built with dune-rpc-lwt would crash when closing
  their connection to the server (ocaml/dune#7581, @gridbugs)

- Introduce mdx stanza 0.4 requiring mdx >= 2.3.0 which updates the default
  list of files to include `*.mld` files (ocaml/dune#7582, @Leonidas-from-XIV)

- Fix RPC server on Windows (used for OCaml-LSP). (ocaml/dune#7666, @nojb)

- Coq language versions less 0.8 are deprecated, and will be removed
  in an upcoming Dune version. All users are required to migrate to
  `(coq lang 0.8)` which provides the right semantics for theories
  that have been globally installed, such as those coming from opam
  (@ejgallego, @Alizter)

- Bump minimum version of the dune language for the melange syntax extension
  from 3.7 to 3.8 (ocaml/dune#7665, @jchavarri)
@aantron
Copy link
Collaborator

aantron commented Dec 11, 2023

As I understand it, public_headers as implemented here is not sufficient for Luv.

I have a library in src/c that needs to install headers from src/c/vendor/include/** such that they are accessible inside a monorepo through -I flags passed by Dune to sibling packages that have C code that depends on the library in src/c without having to include the vendor/include path fragment inside #include. That is, I need either the headers to be copied to the install directory for the library in src/c, or -I that-directory/vendor/include to be passed to the C bindings in the sibling package. Doing dune build shows that these flags are not passed to the C code being compiled in the sibling library, and, predictably, that library cannot find the headers.

@aantron
Copy link
Collaborator

aantron commented Dec 11, 2023

More information here: #4121 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alizter Alizter Alizter left review comments

@anmonteiro anmonteiro anmonteiro approved these changes

@emillon emillon emillon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.

-I option not passed during development for headers installed by a sibling package from a monorepo
5 participants
@rgrinberg @anmonteiro @aantron @emillon @Alizter