-
Notifications
You must be signed in to change notification settings - Fork 415
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 defining libs with same name in multiple contexts (alt implementation) #10296
Allow defining libs with same name in multiple contexts (alt implementation) #10296
Conversation
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
fc3c036
to
c9d77c8
Compare
let expander = Expander0.get ~dir in | ||
Library.to_lib_info s.old_name ~expander ~dir ~lib_config |> Lib_info.of_local | ||
in | ||
let* enabled = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this approach is that you're now evaluating the enabled_if
so early, that you will get a cycle anytime you use a non trivial expression in enabled_if
. Sure that works if you just select based on the context, but if you do something like:
(library
(name foo)
(enabled_if %{read:foo}))
(rule (with-stdout-to foo (echo true))
You will now get a cycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I certainly didn't think about this case. I understand it's a quite edgy case, as it was not possible to use read
variable in enabled_if
until a few days ago when you added support for that.
I opened #10298 to understand what the current behavior is in main
. Then I ran the same test in this branch. The cycle is identified correctly here too, but there is some difference in the report message, with some lines missing in this branch:
Error: Dependency cycle between:
- library "foo" in _build/default
-> %{read:foo} at dune:3
- -> library "foo" in _build/default
So in this branch, the message doesn't include the library
lines, but users can still see there's a cycle and get pointed to the precise file and line where it's happening.
All things considered, my opinion is that the approach used in this PR is a lot simpler than the other one, and introducing a lot of complexity to keep this error message is not worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I ran the same test in this branch.
See 815cc56.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error messages are not the point and this isn't an edge case. The final goal is to lift such limitations from enabled_if, and this PR is going the other way in that regard.
Signed-off-by: Javier Chávarri <[email protected]>
Continues in #10307. |
This PR adds the same feature as #10179 —enabling "multi-context" libraries— but following an alternate implementation.
In #10179 we attempted to defer the resolution of the
enabled_if
field in libraries as much as possible, until the lib databases are queried. This led to a few issues, the most important ones being:main
, which handles the duplicated public or private libraries pretty wellIn this PR, we move the resolution of
enabled_if
field to the library databases creation. This works because even in a multi-context world, there following invariant still exists:There can be at most 1 library behind a given name under a given context.
Fortunately, different library databases are created for different contexts, so by discerning between enabled and disabled libraries, we can just ignore disabled ones when building each lib db.
Most of the diff is about moving the
Memo.t
monad inside thecreate_db_from_stanzas
function, and everything that entailed. There are two parts that were brought along from #10179 as they were:enabled_in_context
inGen_rules
when handling theLibrary
caseeif*
tests that verifies the case of mixing public and private libsCommits:
eif*
tests due to attempts to create rules for libraries that are disabled (and not added to the DB anymore)