Skip to content
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

Fix locating program in exec watch mode #10386

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Apr 4, 2024

This fixes a bug where running an executable from the current project
with dune exec --watch would be unable to find the executable unless
the command was run from the project's root directory.

The problem was introduced when we started setting the cwd of processes
spawned by exec in watch mode to the user's current directory
(#10262). If the program argument to
exec refers to a file to be built in the current project (such as an
executable implemented in the current project) then the path to the
executable to spawn will be a path inside the _build directory relative
to the project root. Since the cwd of the new process was set to the
user's current directory, this relative path was being resolved within
the current directory, whereas it should have been resolved relative to
the project root.

The fix was to convert relative paths into absolute paths relative to
the project root (this was already being done for exec when not in watch
mode).

@@ -0,0 +1,34 @@
Test the behaviour of exec watch mode when the current directory is not the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a repro for this issue, right? can you land it first in its own PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a new pr here: #10391

bin/import.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens with cd path && dune exec -- echo hello?

IIUC, echo would pass the Filename.is_relative check and get a prefix when it shouldn't?

@gridbugs
Copy link
Collaborator Author

gridbugs commented Apr 5, 2024

what happens with cd path && dune exec -- echo hello?

IIUC, echo would pass the Filename.is_relative check and get a prefix when it shouldn't?

dune exec converts paths with no slashes into absolute paths by looking them up in PATH, so any program names will be represented by their absolute path here.

@gridbugs gridbugs force-pushed the exec-watch-relative-path-fix branch 2 times, most recently from 2774e94 to 27a4ed5 Compare April 5, 2024 03:40
This fixes a bug where running an executable from the current project
with `dune exec --watch` would be unable to find the executable unless
the command was run from the project's root directory.

The problem was introduced when we started setting the cwd of processes
spawned by exec in watch mode to the user's current directory
(ocaml#10262). If the program argument to
exec refers to a file to be built in the current project (such as an
executable implemented in the current project) then the path to the
executable to spawn will be a path inside the _build directory relative
to the project root. Since the cwd of the new process was set to the
user's current directory, this relative path was being resolved within
the current directory, whereas it should have been resolved relative to
the project root.

The fix was to convert relative paths into absolute paths relative to
the project root (this was already being done for exec when not in watch
mode).

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs gridbugs force-pushed the exec-watch-relative-path-fix branch from 27a4ed5 to 627cd51 Compare May 29, 2024 00:45
@gridbugs
Copy link
Collaborator Author

Just making sure that we don't forget to merge this. Are any other changes necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants