It seems like you pass the toggle to artifacts_obj but you don't do anything with it, any reason for that?

I used it originally, but I guess I can remove it now and just pass the right list of libs, exes and emits directly from the caller, if that's the better approach.

I'm not sure if it's the better approach, so I'm looking at the code right to find out. I'll list my thought process below. You'll have to repeat in a few different places, so it might be useful.

First, I need to check where are the callers of Artifacts_obj that use a library name. Looks like there's only one caller of:

val lookup_library : t -> Lib_name.t -> Lib_info.local option

That lives in Expander:

    (match Artifacts_obj.lookup_library artifacts name with
     | None -> does_not_exist ~what:"Library" (Lib_name.to_string name)
     | Some lib ->
       Mode.Dict.get (Lib_info.archives lib) mode
       |> Action_builder.List.map ~f:(fun fn ->
         let fn = Path.build fn in
         let+ () = Action_builder.path fn in
         Value.Path fn))

So now I think about what's going to happen if we pretend that a disabled library does not exist. Since this code is part of the expander, to reproduce this behavior I'd run the following command dune build %{cma:dir/disabled-lib}.

From the perspective of the user, dune telling me this does not exist would be rather undesirable. So we need to improve the situation somehow. To do so, we need Artifacts_obj.lookup_library to propagate the library being disabled.

I make the conclusion that your decision is indeed correct and we need to pass enabled/disabled to Artifacts_obj

Hope that helps.

Just to make sure I understand, this process would help on the eventual case that in some future Dune sets "fake" rules for all the disabled libraries in the current context right? As we confirmed the other day that there's no need to set rules for disabled libs right now.

I also wonder, in this hypothetic future where there are fake rules for disabled libraries (for better user errors), how would we deal with the case where two libraries are defined in the same folder with the same name, but one defined on each context? I assume we would skip the creation of fake rules in that case?

Yes, we discussed using fake rules to make the error messages better. And indeed the decision was to forego them for now, as it's quite complex to implement. However, there's other cases where we can improve the error messages without much complexity. Expanding percent forms like we have here is an obvious candidate.

In your hypothetical case we would skip the fake rules. As they would overlap with actual rules.

I added a test case for this in #10245

Looks like we don't handle disabled libraries correctly

The change requests didn't specify if this is something that should be fixed in this PR. Is that the case? In any case, the PR doesn't seem to change the current behavior.

No, there's no need to fix it here.

cc @emillon. This PR seems to fix the case found in #7899.

The old behavior was in fact correct and this PR breaks it. The description of this test might be the source of confusion. There's no limitation anywhere, it's just that the different semantics were chosen for enabled_if on tests.

Should be fixed in 050aed8.

Just as a side note, if I'm understanding the current behavior properly, it is quite confusing from a user perspective. enabled_if semantics are now different depending on where it's used:

• In executable stanzas, the field means the executable is not built if it evaluates to false
• In test stanzas:
  • when all alias is used, the field means the test executable is always built, regardless its evaluated value
  • but for runtest alias it means the executable will not be built if it evaluates to false (like executable stanzas)

I found this comment that explains the use case for it: #7899 (comment).

It's probably too late, but wonder if it would have been possible to leave the behavior of enabled_if consistent, and allow advanced use cases to use a combination of rule and executable, or add a new field test_if.

Yes, in hindsight it would be better to do it your way. We could revisit it for 4.0

These errors might be more meaningful to users than the previous behavior (not building anything because the alias is "empty", but not erroring either).

is it time we make this a record instead? that'd be a whole lot easier to follow what each member of this group_part is.

I agree. I can do so in a follow-up PR.

The old behavior was in fact correct and this PR breaks it. The description of this test might be the source of confusion. There's no limitation anywhere, it's just that the different semantics were chosen for enabled_if on tests.

I added a test case for this in #10245

Looks like we don't handle disabled libraries correctly