-
Notifications
You must be signed in to change notification settings - Fork 456
Nested dune call fix (executable resolution part) #12902
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
base: main
Are you sure you want to change the base?
Nested dune call fix (executable resolution part) #12902
Conversation
Why is it independent? I think if you were to insert dune in the PATH correctly, it would be enough to respect this PATH when doing binary look ups. I think it would make this work redundant. |
Inserting dune in
To further address the issues in 2 we need to probably create a temporary home for The technical details of the temporary directory need to be correctly sorted out. i.e. where does it live? Should be on the same file system, should be hardlinked etc. |
f06d1d1 to
136f99b
Compare
|
Discussing this with @Alizter we noticed that we need a way to call a specific dune (the dune under test) but in a way where It turns out our initial solution |
|
We still need to check that this does something sane on the platforms we are interested in, but if it turns out not to be the case, I think introducing our own way of determining this is better. Generally relying on |
The way the test is implemented, the code testing dune is always calling the (outside, Dune unter test) via its absolute path. This change, extracted from ocaml#12902 calls Dune via absolute path but wraps it in an untility where argv[0] is set to "dune" instead of the absolute path, more closely resembling a call to `dune build` instead of (a hypothetical) `/usr/bin/dune build`. Signed-off-by: Marek Kubica <marek@tarides.com>
The way the test is implemented, the code testing dune is always calling the (outside, Dune unter test) via its absolute path. This change, extracted from ocaml#12902 calls Dune via absolute path but wraps it in an utility where argv[0] is set to "dune" instead of the absolute path, more closely resembling a call to `dune build` instead of (a hypothetical) `/usr/bin/dune build`. Signed-off-by: Marek Kubica <marek@tarides.com>
The way the test is implemented, the code testing dune is always calling the (outside, Dune unter test) via its absolute path. This change, extracted from ocaml#12902 calls Dune via absolute path but wraps it in an utility where argv[0] is set to "dune" instead of the absolute path, more closely resembling a call to `dune build` instead of (a hypothetical) `/usr/bin/dune build`. Signed-off-by: Marek Kubica <marek@tarides.com>
The way the test is implemented, the code testing dune is always calling the (outside, Dune unter test) via its absolute path. This change, extracted from ocaml#12902 calls Dune via absolute path but wraps it in an utility where argv[0] is set to "dune" instead of the absolute path, more closely resembling a call to `dune build` instead of (a hypothetical) `/usr/bin/dune build`. Signed-off-by: Marek Kubica <marek@tarides.com>
The way the test is implemented, the code testing dune is always calling the (outside, Dune unter test) via its absolute path. This change, extracted from ocaml#12902 calls Dune via absolute path but wraps it in an utility where argv[0] is set to "dune" instead of the absolute path, more closely resembling a call to `dune build` instead of (a hypothetical) `/usr/bin/dune build`. Signed-off-by: Marek Kubica <marek@tarides.com>
The way the test is implemented, the code testing dune is always calling the (outside, Dune unter test) via its absolute path. This change, extracted from ocaml#12902 calls Dune via absolute path but wraps it in an utility where argv[0] is set to "dune" instead of the absolute path, more closely resembling a call to `dune build` instead of (a hypothetical) `/usr/bin/dune build`. Signed-off-by: Marek Kubica <marek@tarides.com>
The way the test is implemented, the code testing dune is always calling the (outside, Dune unter test) via its absolute path. This change, extracted from ocaml#12902 calls Dune via absolute path but wraps it in an utility where argv[0] is set to "dune" instead of the absolute path, more closely resembling a call to `dune build` instead of (a hypothetical) `/usr/bin/dune build`. Signed-off-by: Marek Kubica <marek@tarides.com>
…12916) The way the test is implemented, the code testing dune is always calling the (outside, Dune unter test) via its absolute path. This change, extracted from #12902 calls Dune via absolute path but wraps it in an utility where argv[0] is set to "dune" instead of the absolute path, more closely resembling a call to `dune build` instead of (a hypothetical) `/usr/bin/dune build`.
136f99b to
2ab67c1
Compare
2ab67c1 to
69e426b
Compare
This is a companion PR to #12902. Where #12902 resolves the `dune` binary to be called to be correct, this PR prepends an extra entry to the `PATH` variable so the environment will point to the right dune instead of whatever was captured in the environment (potentially by accident). It has a non-essential dependency on #12916 to show that the Dune binary is correctly resolved in all cases.
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
69e426b to
034d29d
Compare
|
I've rebased this on main as #12935 was merged. It is the remaining piece to get #10019 fixed. The reason why this PR is independent from the other is because Dune does the resolution of the program executable in one place ( As the |
Sudha247
left a comment
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.
This looks like a sensible fix!
#10019 mentions that Dune should run itself when encountering package rules, which is quite sensible to do. This PR fixes part of the problem - the
runcommand resolution. When encountering a command dune attempts to look up the programs path. But if the command isdune, we can immediately resolve this to the current executable (was we are Dune, even if the command is not calleddunebutdune.exeormain.exeor something else). This prevents capture of an accidental dune executable (as exhibited by the repro fix).This PR thus solves the first case of #10020, where
duneis called directly. It does not fix the second part (dune not part of the path), which requires a different fix that I intend to submit as a separate PR (as the fix is somewhat independent from this one).