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.

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

Private libraries attached to a package #3655

Merged
merged 5 commits into from
Oct 13, 2020

Conversation

rgrinberg
Copy link
Member

@rgrinberg rgrinberg commented Jul 26, 2020

This PR implements the (package ..) field on libraries that lack a public name. For example:

(library
 (name foo)
 (package bar))

This makes foo available to the public libraries in bar but not to public libraries in any other package. foo will be installed as bar.__private__.foo. The name mangling encourages users to avoid using this library name directly.

I have a some questions about the implementation to @emillon and @jeremiedimino:

Initially, we agreed that the cmi's of such libraries will not be installed. I'm not entirely sure this is workable however. Consider this setup with 2 libraries and an executable in a single project

(library
 (name foo)
 (package bar))

(library
 (libraries foo)
 (public_name bar))

(executable
 (name baz)
 (libraries bar)
 (package baz))

Suppose that (implicit_transitive_deps true) is set. Do you expect the executable baz to see the cmi's of foo? If you think the answer is negative, then consider the situation if the executable was now:

(executable
 (name baz)
 (libraries bar foo)
 (package baz))

This definition is perfectly reasonable with the private libraries we have today, but to make it work with a package-private foo, we need to make sure the installed cmi's of foo are used. That's because bar is installed and we must build against the same version of foo that bar used.

Alternatively, we could simply disallow the usage of such libraries by executables from other packages. Though i think this is a serious limitation.

Progress

  • Mangle names of libraries when installing them
  • Make private packge names visible when they are hidden by -p.
  • Allow libraries in the same scope to refer to such libraries.
  • Hide cmi's of such package private libraries from findlib users
  • Install cmi's of package private libraries into a separate directory
  • Hide cmi's of such package private libraries from users outside of the scope.
  • Document package private libraries
  • Restrict it to 2.8 and above.
  • Fix overlap checks
  • Implement name mangling for package private libraries.

@rgrinberg rgrinberg requested review from emillon and a user July 26, 2020 20:40
@ghost
Copy link

ghost commented Jul 27, 2020

Alternatively, we could simply disallow the usage of such libraries by executables from other packages. Though i think this is a serious limitation.

We agreed to keep this limitation on slack the other day. So let's do that unless there is a new argument in favour of lifting this restriction.

@ghost ghost assigned emillon Jul 27, 2020
@rgrinberg
Copy link
Member Author

We agreed to keep this limitation on slack the other day. So let's do that unless there is a new argument in favour of lifting this restriction.

Not exactly. We talked about the limitation for libraries. I.e. we agreed that library x cannot depend on a private library y in package foo, unless x is also in foo. That seems fine because it was completely impossible for public libraries to depend on private libs, so there was no limitation introduced. However, for executables, private libraries that belong to a package are now strictly less powerful. As normal private libraries can be used in executables freely.

Anyway, I'll introduce the restriction for executables as well then. I don't think it's such a big deal for v1.

@rgrinberg
Copy link
Member Author

So let's do that unless there is a new argument in favour of lifting this restriction.

Actually, there's a new argument: I don't know how to lift this restriction.

The issue with executables is that it's not easy at all to know which package they belong to. The following case is trivial:

(executable (name foo) (package bar))

But what about this one:

(executable (name foo))
(install (section bin) (package bar) (files foo.exe))

@ghost
Copy link

ghost commented Jul 28, 2020

The executable case is more convincing. Agreed that this is not ideal.

If we install the cmi files, then adding (package foo) to private library lib becomes exactly the same as adding (public_name foo.__private__.<libname>), right?

@rgrinberg
Copy link
Member Author

Almost. The cmi's for the private library aren't going to be visible if you use them in an executable/library that lives in another project. Since these cmi's are going to live in a separate dir.

In fact we should probably disallow the mangled names from being written by the user altogether. We really only need them for compatibility with findlib anyway.

@ghost
Copy link

ghost commented Jul 29, 2020

The cmi's for the private library aren't going to be visible if you use them in an executable/library that lives in another project.

I can see how that would work in a mono-repo, but how would that work with opam? If we consider this example:

(library
 (name foo)
 (package bar))

(executable
 (name baz)
 (libraries foo)
 (package baz))

Once package bar is installed and we are compiling package baz though opam, so with -p baz, how will Dune know that bar is part of the same package?

In fact, there is another problem: when building package baz, library foo will be hidden given that it is part of another package. So the library name foo will not exist. How will Dune resolve this name?

@rgrinberg
Copy link
Member Author

Once package bar is installed and we are compiling package baz though opam, so with -p baz, how will Dune know that bar is part of the same package?

In fact, there is another problem: when building package baz, library foo will be hidden given that it is part of another package. So the library name foo will not exist. How will Dune resolve this name?

We could change how -p works. Before filtering any stanzas, we need to construct a map of packages to library names for every dune-project. My only concern is that it will somehow harm performance in some unexpected way. But I can't think of how because we already need to resolve all the libraries in a scope before evaluating rules.

Alternatively, we could require the user to qualify such dependencies by the package:

(executable
 (name baz)
 (libraries (package bar (foo))
 (package baz))

This will only be allowed if bar and baz share the same scope.

@ghost
Copy link

ghost commented Jul 30, 2020

We could also do what we did for variants: instead of filtering we replace the stanza by something like (library_alias (name foo) (libraries bar.__private__.foo)).

Note that there is also a conceptual difference with the original proposal: with the original proposal the developer has a stronger guarantee that its users won't try to use the private libraries. Here, we only get the guarantee for users using Dune, but someone using ocamlfind could happily go and use such private libraries.

@rgrinberg
Copy link
Member Author

We could also do what we did for variants: instead of filtering we replace the stanza by something like (library_alias (name foo) (libraries bar.private.foo)).

Yeah, that seems good.

Note that there is also a conceptual difference with the original proposal: with the original proposal the developer has a stronger guarantee that its users won't try to use the private libraries. Here, we only get the guarantee for users using Dune, but someone using ocamlfind could happily go and use such private libraries.

It wouldn't be enough because using that name in findlib would only make the object files visible. The.cmi files would live in a sub directory. They would need something hacky like ocamlfind ocamlc -I $(ocamlfind query bar.__private__.foo)/.private. If users are willing to go through this trouble, we might as well let them.

@ghost
Copy link

ghost commented Jul 30, 2020

I see. Yes, in this case that all looks good.

@rgrinberg rgrinberg added this to the 2.8 milestone Aug 13, 2020
@rgrinberg rgrinberg force-pushed the private-lib-package branch 2 times, most recently from c96f58c to b2d257a Compare August 29, 2020 09:44
@rgrinberg rgrinberg force-pushed the private-lib-package branch 3 times, most recently from 00372fa to fc3ff23 Compare September 15, 2020 14:49
@rgrinberg rgrinberg mentioned this pull request Sep 22, 2020
rgrinberg added a commit that referenced this pull request Sep 23, 2020
@rgrinberg rgrinberg force-pushed the private-lib-package branch 2 times, most recently from a2826ac to 211dc04 Compare September 30, 2020 08:09
@rgrinberg
Copy link
Member Author

@bobot this PR is ready for review.

I will add name mangling in a separate PR, as I'd like to do a bit more refactoring + work on the (formerly named) extra_aliases first.

Copy link
Collaborator

@bobot bobot left a comment

Choose a reason for hiding this comment

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

That's great. It simplifies a lot the mental model of what is possible or not and open a lot the possibilities.

src/dune_rules/lib.ml Show resolved Hide resolved
@bobot
Copy link
Collaborator

bobot commented Oct 8, 2020

Just why do we accept for executable of another package but in the same project to depend on a private library and not for libraries?

@rgrinberg rgrinberg force-pushed the private-lib-package branch 2 times, most recently from 00411c5 to 3777712 Compare October 13, 2020 00:06
@rgrinberg rgrinberg requested a review from nojb as a code owner October 13, 2020 00:06
@rgrinberg
Copy link
Member Author

Just why do we accept for executable of another package but in the same project to depend on a private library and not for libraries?

Libraries may depend on any private library inside the project as long as it has (package ..). Previously, we tried to limit it to the package, but lifted this restriction exactly for this reason. We wanted it to be symmetric for executables.

Allows us to add (package ..) to private libraries

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Move library redirect creation back to [Gen_rules]. While this is less
elegant, the previous check in [Scope] was much more fragile. It relied
on a name collision to always be caused by a redirect, which is not
something that might be guaranteed in the future.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
[From_project] can just be [From_same_project]. There's no need to check
if they're from the same project, because the library wouldn't be
reachable via the private name anyway.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg merged commit b105c8f into ocaml:master Oct 13, 2020
@rgrinberg rgrinberg deleted the private-lib-package branch October 13, 2020 21:17
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jan 13, 2021
…ne-action-plugin, dune-private-libs and dune-glob (2.8.0)

CHANGES:

- `dune rules` accepts aliases and other non-path rules (ocaml/dune#4063, @mrmr1993)

- Action `(diff reference test_result)` now accept `reference` to be absent and
  in that case consider that the reference is empty. Then running `dune promote`
  will create the reference file. (ocaml/dune#3795, @bobot)

- Ignore special files (BLK, CHR, FIFO, SOCKET), (ocaml/dune#3570, fixes ocaml/dune#3124, ocaml/dune#3546,
  @ejgallego)

- Experimental: Simplify loading of additional files (data or code) at runtime
  in programs by introducing specific installation sites. In particular it allow
  to define plugins to be installed in these sites. (ocaml/dune#3104, ocaml/dune#3794, fixes ocaml/dune#1185,
  @bobot)

- Move all temporary files created by dune to run actions to a single directory
  and make sure that actions executed by dune also use this directory by setting
  `TMPDIR` (or `TEMP` on Windows). (ocaml/dune#3691, fixes ocaml/dune#3422, @rgrinberg)

- Fix bootstrap script with custom configuration. (ocaml/dune#3757, fixes ocaml/dune#3774, @marsam)

- Add the `executable` field to `inline_tests` to customize the compilation
  flags of the test runner executable (ocaml/dune#3747, fixes ocaml/dune#3679, @lubegasimon)

- Add `(enabled_if ...)` to `(copy_files ...)` (ocaml/dune#3756, @nojb)

- Make sure Dune cleans up the status line before exiting (ocaml/dune#3767,
  fixes ocaml/dune#3737, @alan-j-hu)

- Add `{gitlab,bitbucket}` as options for defining project sources with `source`
  stanza `(source (<host> user/repo))` in the `dune-project` file.  (ocaml/dune#3813,
  @rgrinberg)

- Fix generation of `META` and `dune-package` files when some targets (byte,
  native, dynlink) are disabled. Previously, dune would generate all archives
  for regardless of settings. (ocaml/dune#3829, ocaml/dune#4041, @rgrinberg)

- Do not run ocamldep to for single module executables & libraries. The
  dependency graph for such artifacts is trivial (ocaml/dune#3847, @rgrinberg)

- Fix cram tests inside vendored directories not being interpreted correctly.
  (ocaml/dune#3860, fixes ocaml/dune#3843, @rgrinberg)

- Add `package` field to private libraries. This allows such libraries to be
  installed and to be usable by other public libraries in the same project
  (ocaml/dune#3655, fixes ocaml/dune#1017, @rgrinberg)

- Fix the `%{make}` variable on Windows by only checking for a `gmake` binary
  on UNIX-like systems as a unrelated `gmake` binary might exist on Windows.
  (ocaml/dune#3853, @kit-ty-kate)

- Fix `$ dune install` modifying the build directory. This made the build
  directory unusable when `$ sudo dune install` modified permissions. (fix
  ocaml/dune#3857, @rgrinberg)

- Fix handling of aliases given on the command line (using the `@` and `@@`
  syntax) so as to correctly handle relative paths. (ocaml/dune#3874, fixes ocaml/dune#3850, @nojb)

- Allow link time code generation to be used in preprocessing executable. This
  makes it possible to use the build info module inside the preprocessor.
  (ocaml/dune#3848, fix ocaml/dune#3848, @rgrinberg)

- Correctly call `git ls-tree` so unicode files are not quoted, this fixes
  problems with `dune subst` in the presence of unicode files. Fixes ocaml/dune#3219
  (ocaml/dune#3879, @ejgallego)

- `dune subst` now accepts common command-line arguments such as
  `--debug-backtraces` (ocaml/dune#3878, @ejgallego)

- `dune describe` now also includes information about executables in addition to
  that of libraries. (ocaml/dune#3892, ocaml/dune#3895, @nojb)

- instrumentation backends can now receive arguments via `(instrumentation
  (backend <name> <args>))`. (ocaml/dune#3906, ocaml/dune#3932, @nojb)

- Tweak auto-formatting of `dune` files to improve readability. (ocaml/dune#3928, @nojb)

- Add a switch argument to opam when context is not default. (ocaml/dune#3951, @tmattio)

- Avoid pager when running `$ git diff` (ocaml/dune#3912, @AltGr)

- Add `(root_module ..)` field to libraries & executables. This makes it
  possible to use library dependencies shadowed by local modules (ocaml/dune#3825,
  @rgrinberg)

- Allow `(formatting ...)` field in `(env ...)` stanza to set per-directory
  formatting specification. (ocaml/dune#3942, @nojb)

- [coq] In `coq.theory`, `:standard` for the `flags` field now uses the
  flags set in `env` profile flags (ocaml/dune#3931 , @ejgallego @rgrinberg)

- [coq] Add `-q` flag to `:standard` `coqc` flags , fixes ocaml/dune#3924, (ocaml/dune#3931 , @ejgallego)

- Add support for Coq's native compute compilation mode (@ejgallego, ocaml/dune#3210)

- Add a `SUFFIX` directive in `.merlin` files for each dialect with no
  preprocessing, to let merlin know of additional file extensions (ocaml/dune#3977,
  @vouillon)

- Stop promoting `.merlin` files. Write per-stanza Merlin configurations in
  binary form. Add a new subcommand `dune ocaml-merlin` that Merlin can use to
  query the configuration files. The `allow_approximate_merlin` option is now
  useless and deprecated. Dune now conflicts with `merlin < 3.4.0` and
  `ocaml-lsp-server < 1.3.0` (ocaml/dune#3554, @voodoos)

- Configurator: fix a bug introduced in 2.6.0 where the configurator V1 API
  doesn't work at all when used outside of dune. (ocaml/dune#4046, @aalekseyev)

- Fix `libexec` and `libexec-private` variables. In cross-compilation settings,
  they now point to the file in the host context. (ocaml/dune#4058, fixes ocaml/dune#4057,
  @TheLortex)

- When running `$ dune subst`, use project metadata as a fallback when package
  metadata is missing. We also generate a warning when `(name ..)` is missing in
  `dune-project` files to avoid failures in production builds.

- Remove support for passing `-nodynlink` for executables. It was bypassed in
  most cases and not correct in other cases in particular on arm32.
  (ocaml/dune#4085, fixes ocaml/dune#4069, fixes ocaml/dune#2527, @emillon)

- Generate archive rules compatible with 4.12. Dune longer attempt to generate
  an archive file if it's unnecessary (ocaml/dune#3973, fixes ocaml/dune#3766, @rgrinberg)

- Fix generated Merlin configurations when multiple preprocessors are defined
  for different modules in the same folder. (ocaml/dune#4092, fixes ocaml/dune#2596, ocaml/dune#1212 and
  ocaml/dune#3409, @voodoos)

- Add the option `use_standard_c_and_cxx_flags` to `dune-project` that 1.
  disables the unconditional use of the `ocamlc_cflags` and `ocamlc_cppflags`
  from `ocamlc -config` in C compiler calls, these flags will be present in the
  `:standard` set instead; and 2. enables the detection of the C compiler family
  and populates the `:standard` set of flags with common default values when
  building CXX stubs. (ocaml/dune#3875, ocaml/dune#3802, fix ocaml/dune#3718 and ocaml/dune#3528, @voodoos)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants