test: demonstrate regression depending on a ppx by its private name #7553
Conversation
1def7ba to
a404a79
Compare
a404a79 to
21502c4
Compare
|
@rgrinberg I pushed a fix for this, guaranteeing that we return the same scope if there's no host scope. |
21502c4 to
19155f0
Compare
19155f0 to
52126ff
Compare
src/dune_rules/super_context.ml
Outdated
| | Some context_host -> | ||
| let+ scope = Scope.DB.find_by_dir context_host.build_dir in | ||
| Some scope | ||
| in |
There was a problem hiding this comment.
There's another piece of code in this file that looks suspicious:
let expander_for_artifacts ~scope ~external_env ~root_expander ~dir =
Expander.extend_env root_expander ~env:external_env
|> Expander.set_scope ~scope |> Expander.set_dir ~dir
It doesn't seem right to set the scope without setting the scope_host appropriately. I guess we can fix this on another occasion.
There was a problem hiding this comment.
I was just typing this in another comment.
There was a problem hiding this comment.
I think the current fix is in the wrong place.
There was a problem hiding this comment.
There's a simpler way to derive the scope_host from the scope that perhaps we should follow:
- Get the root directory of
scope. - Transform this directory to the analogous directory in the host's context
- Call
Scope.DB.find_by_diron it.
This kind of complicates my refactoring of making source tree context based, but it's ok for now.
There was a problem hiding this comment.
So for this change, you would need a ; context_host : Context.t option field rather than a scope_host.
There was a problem hiding this comment.
Or perhaps we should modify set_scope to take both the scope and the scope_host. and move this logic to whoever is calling set_scope. It seems good to keep Expander relatively oblivious to cross compilation logic.
There was a problem hiding this comment.
I pushed the version where scope_host is derived from context. I'll explore the version where set_scope takes scope_host now and see if I like it better.
There was a problem hiding this comment.
Pushed the set_scope fix instead, which is a lot more minimal. Let me know what you think.
c0a4ca7 to
9fa4123
Compare
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
9fa4123 to
5981ac5
Compare
|
I wonder if we have a similar issue for artifacts. |
|
Looks like dune/src/dune_rules/expander.ml Line 85 in 82c670e |
ppxlib.traversewhen ppxlib is vendored #7543 and is a lighter weight alternative to Add repro for github issue 7543 #7544