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

Allow libraries without archives #3973

Merged
merged 9 commits into from
Jan 13, 2021
Merged

Conversation

rgrinberg
Copy link
Member

Before we fix this bug, we need proper tests.

@kit-ty-kate are there any other cases?

@rgrinberg rgrinberg requested a review from nojb November 24, 2020 23:00
@kit-ty-kate
Copy link
Member

kit-ty-kate commented Nov 24, 2020

Apart from adding (used in digestif.0.7.1, see #3904 (comment)):

(library
  (public_name foo)
  (wrapped false)
  (modules_without_implementation foo))

I don't see any other tests to add to that for now.

Thanks a lot for doing that!

@rgrinberg
Copy link
Member Author

Apart from adding (used in digestif.0.7.1, see #3904 (comment)):

I'm not yet confident this is the same issue. Let me investigate this separately.

@kit-ty-kate
Copy link
Member

I'm not yet confident this is the same issue. Let me investigate this separately.

It does look like the same category of issue to me, as with master this test-case works with OCaml 4.11 but fails on OCaml 4.12

@rgrinberg
Copy link
Member Author

It does look like the same category of issue to me, as with master this test-case works with OCaml 4.11 but fails on OCaml 4.12

Indeed. I've added it as well.

@rgrinberg rgrinberg force-pushed the 3766-repro branch 2 times, most recently from 1e29831 to e963b11 Compare November 27, 2020 00:38
@dra27
Copy link
Member

dra27 commented Dec 7, 2020

One more case - an externally built library (so providing a META file) may have a .cmxa with no .a/.lib

@rgrinberg rgrinberg force-pushed the 3766-repro branch 2 times, most recently from acc7c97 to 4575e99 Compare December 18, 2020 00:49
@rgrinberg rgrinberg requested a review from dra27 December 18, 2020 00:49
@rgrinberg
Copy link
Member Author

@dra27 do you mind going over the test cases + review the checks in dune_file.ml?

@kit-ty-kate could you please test?

@nojb would you be able to review the rest of the PR?

@rgrinberg
Copy link
Member Author

I ended up discarding the dummy module idea - it doesn't make anything easier after all. If we decide to use a dummy module, we must create an Obj_dir.t that accommodates private modules (or pay a huge performance price to always use the inefficient layout)., but we already need an instance of Obj_dir.t to load the rules and to initialize the library database. Hence we are back at the same cycle that we wanted to avoid in the first place.

In the end, I had to bite the bullet and workaround this cycle. I chose to do it in a truly gruesome manner (my apologies to reviewers) to avoid making this PR even huger and more prone to mistakes. I will make another attempt to cut this gordian cycle between dir_contents, lib, and super_context in another PR.

@@ -834,10 +835,27 @@ module Library = struct
let virtual_library = is_virtual conf in
let foreign_archives = foreign_lib_files conf ~dir ~ext_lib in
let native_archives =
if modes.native then
[ archive ext_lib ]
let archive = archive ext_lib in
Copy link
Member Author

Choose a reason for hiding this comment

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

@dra27 this the sensitive bit.

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Dec 18, 2020

@rgrinberg Thanks a lot! I tried this branch locally and even though the great majority of packages compiled fine, some failed:

#=== ERROR while compiling uuuu.0.2.0 =========================================#
# context     2.1.0~beta4 | linux/x86_64 | ocaml-variants.4.12.0+trunk | git://github.com/ocaml/opam-repository.git
# path        ~/.opam/4.12/.opam-switch/build/uuuu.0.2.0
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p uuuu -j 3
# exit-code   1
# env-file    ~/.opam/log/uuuu-1304759-16c4d6.env
# output-file ~/.opam/log/uuuu-1304759-16c4d6.out
### output ###
#     ocamlopt gen/generate.exe (exit 2)
# (cd _build/default && /home/kit_ty_kate/.opam/4.12/bin/ocamlopt.opt -w -40 -g -o gen/generate.exe /home/kit_ty_kate/.opam/4.12/lib/bigarray-compat/bigarray_compat.cmxa /home/kit_ty_kate/.opam/4.12/lib/bigstringaf/bigstringaf.cmxa -I /home/kit_ty_kate/.opam/4.12/lib/bigstringaf /home/kit_ty_kate/.opam/4.12/lib/angstrom/angstrom.cmxa gen/.generate.eobjs/native/source.cmx gen/.generate.eobjs/nat[...]
# gcc: error: /home/kit_ty_kate/.opam/4.12/lib/bigarray-compat/bigarray_compat.a: No such file or directory
# File "caml_startup", line 1:
# Error: Error during linking (exit code 1)
#=== ERROR while compiling uri.4.0.0 ==========================================#
# context     2.1.0~beta4 | linux/x86_64 | ocaml-variants.4.12.0+trunk | git://github.com/ocaml/opam-repository.git
# path        ~/.opam/4.12/.opam-switch/build/uri.4.0.0
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p uri -j 3
# exit-code   1
# env-file    ~/.opam/log/uri-1304759-db578e.env
# output-file ~/.opam/log/uri-1304759-db578e.out
### output ###
#     ocamlopt config/gen_services.exe (exit 2)
# (cd _build/default && /home/kit_ty_kate/.opam/4.12/bin/ocamlopt.opt -w -40 -g -o config/gen_services.exe /home/kit_ty_kate/.opam/4.12/lib/stringext/stringext.cmxa config/.gen_services.eobjs/native/gen_services.cmx)
# gcc: error: /home/kit_ty_kate/.opam/4.12/lib/stringext/stringext.a: No such file or directory
# File "caml_startup", line 1:
# Error: Error during linking (exit code 1)
#=== ERROR while compiling tls.0.12.8 =========================================#
# context     2.1.0~beta4 | linux/x86_64 | ocaml-variants.4.12.0+trunk | git://github.com/ocaml/opam-repository.git
# path        ~/.opam/4.12/.opam-switch/build/tls.0.12.8
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p tls -j 3
# exit-code   1
# env-file    ~/.opam/log/tls-1304759-5cb026.env
# output-file ~/.opam/log/tls-1304759-5cb026.out
### output ###
#     ocamlopt .ppx/d824e2e5ac48a1dc0db041713fc7f187/ppx.exe (exit 2)
# (cd _build/default && /home/kit_ty_kate/.opam/4.12/bin/ocamlopt.opt -g -w -24 -o .ppx/d824e2e5ac48a1dc0db041713fc7f187/ppx.exe /home/kit_ty_kate/.opam/4.12/lib/ocaml/unix.cmxa -I /home/kit_ty_kate/.opam/4.12/lib/ocaml /home/kit_ty_kate/.opam/4.12/lib/ocaml/bigarray.cmxa -I /home/kit_ty_kate/.opam/4.12/lib/ocaml /home/kit_ty_kate/.opam/4.12/lib/sexplib0/sexplib0.cmxa /home/kit_ty_kate/.opam/4.[...]
# gcc: error: /home/kit_ty_kate/.opam/4.12/lib/ppx_cstruct/ppx_cstruct.a: No such file or directory
# File "caml_startup", line 1:
# Error: Error during linking (exit code 1)

@dra27
Copy link
Member

dra27 commented Dec 18, 2020

I haven't reviewed the code, but I can confirm that with 4.12.0~alpha3 on msvc64, dune master can build stdlib-shims but not run the tests and this PR can both build stdlib-shims and run the tests, thanks! 🎉

@kit-ty-kate
Copy link
Member

After double-checking using opam-health-check, I can confirm this PR is all good to go. None of the packages in opam-repository are now broken.

Thanks a lot! 🎉

@Octachron
Copy link
Member

Asking the question on the right PR this time, what is the status of this PR? From the compiler point of view, the beta release of OCaml 4.12.0 is looming on the horizon, and #3766 is the major remaining blocker. Would it be easier from the point of view of dune team to backport this fix to a hypothetical dune 2.7.2?

@rgrinberg
Copy link
Member Author

Would it be easier from the point of view of dune team to backport this fix to a hypothetical dune 2.7.2?

Releasing 2.8 would be easier as backporting this PR to the 2.7 branch might not be trivial. Once this PR is reviewed & merged, we'll release 2.8 ASAP.

@rgrinberg rgrinberg changed the title Add reproduction case for 3766 Allow libraries without archives Jan 6, 2021
@rgrinberg rgrinberg added this to the 2.8 milestone Jan 7, 2021
src/dune_rules/lib.ml Show resolved Hide resolved
src/dune_rules/dune_file.ml Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM

@rgrinberg rgrinberg force-pushed the 3766-repro branch 2 times, most recently from ad3b098 to 3eae4be Compare January 13, 2021 14:52
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Use a for_ argument instead of separate functions

Signed-off-by: Rudi Grinberg <[email protected]>
Creation of obj_dir requires knowledge about private modules.

Signed-off-by: Rudi Grinberg <[email protected]>
This reverts commit 3ecfd8bd5f13bc0252e771dae599822babafa67b.

Signed-off-by: Rudi Grinberg <[email protected]>
it's no longer necessary

Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg rgrinberg merged commit bfc4fd7 into ocaml:master Jan 13, 2021
@rgrinberg rgrinberg deleted the 3766-repro branch January 13, 2021 15:11
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.

4 participants