Add test to show lib name collision in contexts

[jchavarri]

I am checking Dune workspaces as part of some exploration to find better ways to configure projects that use both Melange and OCaml libraries with some shared code.

My assumption was that a workspace has its own library namespace, and it is not shared with other workspaces. So it would be possible to define a library foo in two workspaces A and B and avoid collisions through the enabled_if field.

As this test shows, this is not the case, and Dune will error out because the library is defined twice.

Is the assumption incorrect? If it isn't, I would be happy to contribute a fix for it to continue this exploration, thanks!

[rgrinberg]

Your test indeed demonstrates a limitation of dune's rules. You're welcome to try and lift this limitation. See scope.ml and lib.ml for how the library database is constructed. It shouldn't be too hard to allow more than one entry per name as long as only one is enabled.

Note that after you lift it, likely it will not be enough because enabled_if on a library accepts very few arguments. That's a different limitation that I would also like to lift.

[jchavarri]

Thanks for the pointers, will take a look.

likely it will not be enough because enabled_if on a library accepts very few arguments. That's a different limitation that I would also like to lift

What kind of new arguments do you have in mind?

[jchavarri]

Sharing here some notes of what I've been able to find out.

Currently, there is a uniform "view" (for lack of a better name) of library dbs that is shared across all contexts. It is created in the Scope.DB.all function, which is called at the very beginning of each Dune run.

Having a uniform view of the library dbs makes it impossible to keep checking for duplication of libraries when Scope.DB.all is called, because at that point we don't have the information yet on the "expander" that is needed to resolve the enabled_if field. So it seems necessary to move the check to a further stage in order to pass the context. Is that right? Otherwise, I think we should change the map used in Scope (which is based on Lib_name) to consider more things besides the library name when finding items in it.

I wonder if rather than using enabled_if for this, there could be a new field for_context added to the library stanza. That way, we could just add the libraries to the DB of the context they belong to "statically", without having to wait for a later stage, or mess with Blang expanders. Similarly to what is done for filtering by packages:

dune/src/dune_rules/scope.ml

Line 336 in 07ba176

let* stanzas = Only_packages.filtered_stanzas (Context.name context) in

The Dune docs only mention os_type in the variables section of enabled_if:

dune/doc/stanzas/library.rst

Lines 193 to 196 in 07ba176

	specified using the :doc:`reference/boolean-language`, and the field allows
	for the ``%{os_type}`` variable, which is expanded to the type of OS being
	targeted by the current build. Its value is the same as the value of the
	``os_type`` parameter in the output of ``ocamlc -config``.

I think enabled_if design is a bit lose, that's why I propose the for_context field, which would settle down conditionally available libraries per context within a more stable design.

[rgrinberg]

Scope.DB.all is computed for all contexts, but it does not create a single database for all the contexts, it creates a database per context. Note the context keyed map it generates.

Otherwise, I think we should change the map used in Scope (which is based on Lib_name) to consider more things besides the library name when finding items in it.

You need to change the map, but it's enough to just make it list valued. Right now it just returns a single Found_or_redirect.t per name, but we should be able to return multiple items here and then filter the list based on the enabled_if value.

Adding specialized fields would make the whole thing more confusing.

[jchavarri]

I'm leaving here some notes, as I have very limited idea of what i'm doing, so maybe they can serve to surface if I'm going in the wrong direction:

• Originally tried to start passing context to the resolve field in Lib.t. This turned out to touch a lot of places, so I suspected it would not be the right way to go, and reverted it
• Found out there's a way to know if a library is enabled using Lib_info.enabled info, so I'm filtering out the results in create_db_from_stanzas map value to only consider the libraries that are enabled.

The second approach seems to be working out, I had to disable some odoc rules due to some internal errors:

Internal error, please report upstream including the contents of _build/log.
Description:
  ("[gen_rules] returned rules in a directory that is not a descendant of the directory it was called for",
  { dir = In_build_dir "alt-context/a"
  ; example =
      Alias
        { dir = In_build_dir "alt-context/b/.foo.objs/byte"
        ; name = ".odoc-all"
        }
  })

I will continue down this path tomorrow, fixing the odoc rules and any other potential breakage after the change.

[rgrinberg]

The second approach seems to be working out, I had to disable some odoc rules due to some internal errors:

It will work out, but it will make the errors worse. Our rules distinguish the following two errors:

1. A library name cannot be resolved because it's not found
2. A library name cannot be resolved because it's not available

Your change would convert all errors of type 2. to type 1.

A better approach is the following:

1. Allow Lib.DB.create to return more than one library per name when the libraries have enabled_if on them (requires modifying Lib.Resolve_result.t)
2. before we call Lib.instantiate_impl, we evaluate the enabled_if of every library that corresponds to our name.
3. If we find that only one enabled_if is true, then we proceed as usual. Otherwise, we complain that more than one library is available.

For inspiration, you can take a look at artifcats.ml where I handle enabled_if correctly but for binaries

[jchavarri]

@rgrinberg Thanks for the pointers, they are super helpful.

I made some progress on the new handling of lib dbs following the steps mentioned (see 1b0c2c3).

Also fixed a cryptic runtime error coming from some Option.value_exn usage (wasn't able to figure out where it was coming from), by moving Odoc.setup_private_library_doc_alias to be called when we know the library is enabled (857ec39). I'm not 100% sure about this, as there was a comment in the main branch, so probably it was kept that way for some reason:

dune/src/dune_rules/gen_rules.ml

Line 115 in c11c1f1

(* XXX why are we setting up private doc rules for disabled libraries? *)

Tomorrow, I'll try to figure out why the .odoc-all alias triggers a returned rules in a directory that is not a descendant of the directory it was called for error, it seems the rules for odoc get added to libraries that should be disabled. I saw that the code in Lib_rules.rules doesn't use resolve_result, but rather Lib.Status.t for some reason, so I might have to look into adapting that type as well to the new situation.

There are also some errors that have regressed, from is hidden (unsatisfied 'enabled_if') to not found. I will look at that later, should be easy to fix 🤞

[rgrinberg]

I've nothing useful to say about odoc issues. It's been a while since I've looked at that stuff. Seems like my comment

(* XXX why are we setting up private doc rules for disabled libraries? *)

is giving us a useful hint. I'd push Odoc.setup_private_library_doc_alias into the subsequent if_available_buildable and see what happens. This can come in a separate PR.

I saw that the code in Lib_rules.rules doesn't use resolve_result, but rather Lib.Status.t for some reason, so I might have to look into adapting that type as well to the new situation.

Lib.Status.t doesn't need to change. Because whoever is constructing the status (resolve_name), should evaluate the enabled_if and error out (by constructing an appropriate Status.Invalid) if more than one library's enabled_if evaluates to true.

[jchavarri]

I have been looking into a weird issue. When I defined a test using a public library using the code in this PR:

  $ cat > dune-project << EOF
  > (lang dune 3.13)
  > (package (name foo))
  > EOF

  $ cat > dune << EOF
  > (library
  >  (name bar)
  >  (public_name foo.bar)
  >  (enabled_if (= %{context_name} "unavailable")))
  > EOF
  $ cat > foo.ml <<EOF
  > let x = "foo"
  > EOF

  $ dune build

The dune build command never ends, because resolve_name function enters an endless loop. Looking at the logic in db.parent, I am not sure I understand how it is supposed to work.

There are two usages of Lib.DB.create:

• in create_db_from_stanzas the parent is public_libs:
  

  dune/src/dune_rules/scope.ml

  Line 249 in c11c1f1

  create_db_from_stanzas stanzas ~instrument_with ~parent:public_libs ~lib_config

• in public_libs the parent is installed_libs:
  

  dune/src/dune_rules/scope.ml

  Line 182 in c11c1f1

  ~parent:(Some installed_libs)

What exactly is db.parent and is there some invariant where it should reflect the parent-child relationship between folders or some other kind of hierarchy? If that's the case, I am not entirely sure it does so at the moment.

[jchavarri]

Unrelated to the comment above (sorry for the unordered stream of comments).

I have some suspicions that this calculation of enabled_if is not correct:

dune/src/dune_rules/gen_rules.ml

Line 117 in 21557d8

and+ enabled_if = Lib.DB.available (Scope.libs scope) (Library.best_name lib) in

The reason is that it does not take the current context into account, so in the test that is included in this PR, it always returns true, even for libraries that should not be enabled under that context. I think this is what is causing the [gen_rules] returned rules in a directory that is not a descendant of the directory it was called for errors in the test.

The reason is that in the case of the test, where there are two libraries with the same name (one for each context) there will always exist some library available with the given name. It seems the accurate way to check if a library is enabled is to get that information using Lib_info.enabled?

[jchavarri]

In 548fabd, I updated the test to show the issue I've been trying to figure out for the last few days.

[rgrinberg]

The reason is that it does not take the current context into account, so in the test that is included in this PR, it always returns true, even for libraries that should not be enabled under that context

A library database is context specific, so it will should account for the when evaluating the availability. See the call to Library.to_lib_info in scope.ml to confirm.

[rgrinberg]

Pushed a fix. The following check for availability was no longer correct:

let* enabled_if = Lib.DB.available (Scope.libs scope) (Library.best_name lib) in

Using your test as an example:

• We are checking the availability of the name foo before building a/foo or b/foo
• The name foo is always available in every context because one of the stanzas will evaluate to true (either a/foo or b/foo)
• So both stanzas are always going to be generated in every context.

The solution is to make the availability check more specific. Instead of checking if the name foo is available, we need to check if foo is available and it resolves to the stanza which we are generating the rules for.

The simplex fix I did addresses the problem, but we should improve the API to make it harder to make this error. E.g.

val available_library_stanza : Lib.DB.t -> dir:Path.Build.t -> Lib_name.t -> bool Memo.t

[jchavarri]

@rgrinberg I added a test for the case you mentioned in #10026.

Re: the issues with get_compile_info failing, here is what I understand is happening.

Let's say there are two contexts default and alt, and a library foo that is only defined in default context through the enabled_if field.

Gen_rules.of_stanza will still call Lib_rules.rules for the stanza that defines foo under alt context, and this is where currently get_compile_info is breaking, because it will not be able to find a library.

I am not sure what Gen_rules.of_stanza is supposed to do when a stanza is disabled for the current context. The function is supposed to return some compilation context + merlin configuration, but what's the point if the stanza is disabled? Shouldn't Dune just don't add any rules in that case and skip the processing of the stanza altogether?

[jchavarri]

@rgrinberg I think I got things to be in a place where all the use cases I wanted to support are working, and nothing existing seems to have regressed (at least from the functionality being tested).

I added some inline notes to help reviewing, the diff might look less noisy without the whitespace.

[jchavarri]

This check might look redundant, but if we don't conditionally return merlin config, the eif-library-name-collision-same-folder test fails with multiple rules for cmxs:

Error: Multiple rules generated for _build/alt-context/foo.cmxs:
- dune:4
- dune:1
Error: Multiple rules generated for _build/default/foo.cmxs:
- dune:4
- dune:1

[jchavarri]

This module was just moved up to make Local available in scope for get_compile_info.

[rgrinberg]

@anmonteiro could you review this one?

[anmonteiro]

are you planning on addressing this limitation too in a future PR?

[jchavarri]

#9839 (comment)

[anmonteiro]

it's a bit weird that this doesn't error?

[anmonteiro]

probably related to them being private libraries, but still weird to me 🤔

[jchavarri]

I agree it's strange. I still not fully understand why this happens yet.

[anmonteiro]

have you thought about how we might make it work for public libraries?

[jchavarri]

I have started an attempt in 619ea9d.

This doesn't work yet, as it seems the result from querying the DB to find a public library is ignored. There's a call to find here:

dune/src/dune_rules/scope.ml

Line 392 in 98c576e

let+ lib = Lib.DB.find public_libs (Public_lib.name pub) in

But this function already skips all invalid results to just return None:

dune/src/dune_rules/lib.ml

Line 1866 in 98c576e

| Ignore | Not_found | Invalid _ | Hidden _ -> None

And then back in the call point, there's a comment that shows that somehow this should be dealt with:

dune/src/dune_rules/scope.ml

Lines 395 to 397 in 98c576e

	(* Skip hidden or unavailable libraries. TODO we should assert
	that the library name is always found somehow *)
	acc

It seems if the result is Invalid (because there are 2 libraries defined) we should do something else?

[anmonteiro]

nit: this only seems to be used once. I'd probably inline it in its usage site.

[jchavarri]

We'll need it for public libs as well (see failed attempt in 619ea9d).

[anmonteiro]

still a TODO here.

a pending question: what if info is the empty list? should we try List.hd info2?

[jchavarri]

there's some Nonempty_list module. Maybe I could add it eventually.

[anmonteiro]

refactoring suggestion:

• rename Lib_info.enabled to Lib_info.to_enabled_if
• add Lib_info.enabled which is val enabled: Lib_info.t -> bool Memo.t
  • implementation is your conversion from the variant to bool (saw that a few times in the PR)

[anmonteiro]

are there unpredictable consequences of getting the lib info before doing the cycle check on its modules?

[jchavarri]

Thanks for the reviews and guidance. This PR got a bit long and messy, it has served its purpose as many parts of it were extracted into separate PRs.

Starting again with a much smaller diff (for now) in #10179.