feature(melange): add correct stdlib dir #6443
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anmonteiro
reviewed
Nov 11, 2022
anmonteiro
reviewed
Nov 11, 2022
anmonteiro
approved these changes
Nov 11, 2022
rgrinberg
force-pushed
the
ps/rr/feature_melange___add_correct_stdlib_dir
branch
2 times, most recently
from
6b85079
to
dfe45e4
Compare
rgrinberg
commented
Nov 11, 2022
|
I was thinking whether this would be correct in watch mode, but I believe there's no way to set an environment variable while the watcher is running. If that's the case, I'm convinced this is correct. |
rgrinberg
commented
Nov 11, 2022
src/dune_rules/melange_rules.ml
Outdated
| let open Memo.O in | ||
| let* env = Super_context.env_node sctx ~dir >>= Env_node.external_env in | ||
| match Env.get env "MELANGELIB" with | ||
| | Some p -> Memo.return (Some (Path.of_string p)) |
There was a problem hiding this comment.
This branch is an optimization I suppose. We can still just run the binary and get the expected result.
Yes, for now there isn't. We have a goal of making Another long term goal is to persist these memoization tables between runs so that restarting dune is fast. |
jchavarri
approved these changes
Nov 11, 2022
src/dune_rules/melange_rules.ml
Outdated
| let open Memo.O in | ||
| let* env = Super_context.env_node sctx ~dir >>= Env_node.external_env in | ||
| match Env.get env "MELANGELIB" with | ||
| | Some p -> Memo.return (Some (Path.of_string p)) |
There was a problem hiding this comment.
for the record, @emillon and I ran into some cases where MELANGELIB would be set to empty string. Not sure if it should be handled here. Or it is enough with melange-re/melange#412.
src/dune_rules/melange_rules.ml
Outdated
| | None -> ( | ||
| let* melc = melc sctx ~dir in | ||
| match melc with | ||
| | Error _ -> Memo.return None |
There was a problem hiding this comment.
Maybe after #6403 we can fail here too? So that melange_where return a non-optional value?
There was a problem hiding this comment.
If we fail, then we wouldn't generate any of the rules in this directory. That would be highly suboptimal
rgrinberg
force-pushed
the
ps/rr/feature_melange___add_correct_stdlib_dir
branch
2 times, most recently
from
dc17733
to
25a4a36
Compare
|
@jchavarri i've fixed a few more issues. give this a try when you have time. |
rgrinberg
force-pushed
the
ps/rr/feature_melange___add_correct_stdlib_dir
branch
from
25a4a36
to
5ffdf8d
Compare
|
I am trying a fork of the melange opam template on the dune branch, pinning both melange and dune: This is what I found: For modules on Maybe expected? For modules inside Melange libraries that use Melange stdlib (e.g.
|
|
@jchavarri could you turn on the verbosity according to ocamllabs/vscode-ocaml-platform#1020 (comment) I'd like to see the error. |
|
|
Thanks. I won't be able to debug this error myself. We'll just notify @voodoos that this issue exists. |
Signed-off-by: Rudi Grinberg <me@rgrinberg.com> ps-id: cce82426-de1a-4473-a3af-1a91ab987de5
rgrinberg
force-pushed
the
ps/rr/feature_melange___add_correct_stdlib_dir
branch
from
5ffdf8d
to
42d15cd
Compare
|
Merging this is already an improvement. Let's keep working on it. |
jchavarri
added a commit
to jchavarri/dune
that referenced
this pull request
Nov 12, 2022
* main: (33 commits) refactor(melange): move [lib_output_dir] (ocaml#6449) refactor: move dune.exe binary to _boot (ocaml#6308) test: split dune-project-meta/main.t (ocaml#6448) feature(melange): add correct stdlib dir (ocaml#6443) feature(melange): virtual libs (ocaml#6444) doc: rewrap dune-files + minor improvements (ocaml#6395) test: split several-packages.t (ocaml#6423) feature(melange): add alias field (ocaml#6401) refactor(melange): share visible_cmi branch (ocaml#6442) fix(ci): build @install to build dune (ocaml#6441) test: split no-installable-mode.t (ocaml#6436) test: split enabled_if-exec.t (ocaml#6439) test: ocamldep-multi-stanzas.t remove nesting (ocaml#6438) test: split duplicate-c-cxx-obj.t (ocaml#6437) test: split bad-alias-error.t (ocaml#6425) test: split duplicate-c-cxx.t (ocaml#6433) test: split intf-only.t (ocaml#6434) fix(coq): fix coqtop args being passed incorrectly test: split github1099.t (ocaml#6435) test: split re-export-exe.t (ocaml#6432) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Rudi Grinberg me@rgrinberg.com
ps-id: cce82426-de1a-4473-a3af-1a91ab987de5