test(melange): demonstrate melange.emit doesn't respect -p

[anmonteiro]

• dune build -p some-package was trying to build melange.emit stanzas belonging to other packages
• the fix is to teach the stanza_package function about the melange package field

[jchavarri]

I found similar code (match over stanzas type) in install_rules.ml (here and here).I wonder if Melange_emit should be included in those cases too.

[rgrinberg]

dune build -p some-package was trying to build melange.emit stanzas belonging to other packages

Isn't that how it works for private executables? Given that there's no way to mark melange.emit as "public", this behavior seems consistent.

[anmonteiro]

So what’s the package field in the emit stanza for?

[anmonteiro]

@rgrinberg FWIW this was found in a real world use case (reason-react). The setup works like this:

• reactjs-ppx.opam defines a ppx that only depends on ppxlib
• reason-react is preprocessed with the react PPX, and defines its own melange tests with melange.emit
• when trying to run dune build -p react-ppx for a release, we get an error in the JS tests (package reason-react not found)

[rgrinberg]

So what’s the package field in the emit stanza for?

No idea tbh. For executables, it's used to attach those executables with public names to packages.

when trying to run dune build -p react-ppx for a release, we get an error in the JS tests (package reason-react not found)

Is it because you're building @all instead of @install? Otherwise, I'm not sure how the melange.emit is being picked up by the @install stanza.

[anmonteiro]

The test in this PR still fails if I change the command to dune build -p my-ppx @install.

Is the issue that melange.emit is being picked up, then?

[rgrinberg]

Actually, it's just an issue with rule loading. The failure comes from the rule loading stage rather than rule execution stage. The issue is that the melange rules use read_memo which blows up when searching for @install dependencies in test/

[anmonteiro]

Interesting. What's the best way to address this?

[rgrinberg]

Resolve.t is just a result type, so you can just inspect it instead of assuming it has a value. In the case where something can't be resolved, we could add some dummy targets that will fail with Action_builder.fail. It's not perfect, but it's the best we can do without proper target masks.

[anmonteiro]

OK, I pushed an alternative fix that also satisfies the test.

I'm not sure 1. what to name the dummy target 2. what to write inside Action_builder.fail, so would appreciate guidance there.

[rgrinberg]

Resolve.to_result can be used here.

[rgrinberg]

1. what to name the dummy target

I would pick some target names that match what we would setup if the dependencies weren't absent. So for melange.emit, that could be the entry point .js files. The intent is to convert "missing rule for target" errors into "missing dependency" errors.

[anmonteiro]

Thank you. The latest push follows that approach.

[Alizter]

Weird that thr Coq tests have lost some location info in the trace.

[rgrinberg]

Mystery is solved

[anmonteiro]

Thanks, Rudi.

[anmonteiro]

@emillon can you port this for 3.8.2 please?

[emillon]

Will do, but this needs a changelog entry.

[anmonteiro]

I don’t think it needs a changes entry since melange support is still 0.1 (we haven’t included melange anywhere in the change log previously)

[emillon]

I don't think it's a hard rule. mdx is 0.x but is present in the changelog for example. Given it's documented now I think future melange changes should get a changelog entry now (and for backports it helps knowing what's been backported)

[rgrinberg]

Didn't we announce melange support in dune? If we did, I think we should include updates in CHANGES. They're informative to users and allow them to follow the development of the feature more easily - even if we aren't yet obligated to maintain backwards compatibility.