-
Notifications
You must be signed in to change notification settings - Fork 407
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
Absolute path exec handling #10857
base: main
Are you sure you want to change the base?
Absolute path exec handling #10857
Conversation
@Khady @rgrinberg Tho this doesnt work indeed if its run from a different dune project(since we cannot resolve the existing builds from different dune project) |
For now i have made the code to preserve the behavior in case of absolute path(where build is not invoked on exec). But i feel it should be same for both relative and absolute. Open for suggestions(the change is just 2 line change only) |
50b4df9
to
f1f2320
Compare
bin/exec.ml
Outdated
Build_system.file_exists path | ||
>>= (function | ||
| true -> Memo.return (Some path) | ||
| false -> | ||
if not (Filename.check_suffix prog ".exe") | ||
then Memo.return None | ||
else ( | ||
let path = Path.extend_basename path ~suffix:".exe" in | ||
Build_system.file_exists path | ||
>>| function | ||
| true -> Some path | ||
| false -> None)) |
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.
I don't know what this code is doing exactly, but this might need to support the .bc
extension too?
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.
I didn't cause any regression so this would a very new thing. Current code has no regression from previous one
thanks for helping with this issue. I'll try to compile and test |
@Khady do you need help in testing this? |
looks like it works for a program in the current directory or a child directory, but not pointing to a parent directory
|
@Khady check this comment #10857 (comment) Is the parent directory also in same project? |
yes it is in the same project. I'm just going down with |
@Khady i will make the changes and push it tonight. Will ping once done |
17fcdaf
to
f6ea52b
Compare
@Khady can you check it now?
The reason is its dependent on the design of dune in itself. |
Now I'm facing a problem where if I use exec of absolute path it does nothing (see line 2) unless I've built first (see line 20 and 22). The behavior of the last 2 commands is also surprising, something different happens depending on if the path is resolved or not.
About the code, it seems that you don't have auto formatting setup in your workflow. So you'll need to run |
@Khady so in the previous version as well there is no build when trying to use absolute path. And for the last 2 commands its not supported since for the executable to be found we need to change to root build dir which is what happening when we include a ".." in the code. Manually it finds and change to the parent build dir |
f6ea52b
to
3e2c530
Compare
I feel like it should be possible to compute the resolved real path in ocaml and make it to work. But I don’t have time to investigate that for now. Will see if I can do it in the following weeks. |
@Khady sure Only thing we are missing now is using the project info from paths excluding basename ryt? |
@Khady i guess i resolved the problem you were mentioning. The current approach changes the root directory to directory mentioned in path
Could you please test this updated one? |
3dd412b
to
6640a06
Compare
Signed-off-by: Athish Pranav D <[email protected]>
Signed-off-by: Athish Pranav D <[email protected]>
Signed-off-by: Athish Pranav D <[email protected]>
5ad1169
to
b6c7da6
Compare
@Khady can you trigger the CI builds? |
31957c1
to
ed1866f
Compare
I am only a contributor to dune, not a maintainer, I don't have the rights to do so (nor to accept and merge your work). |
Oh I see Can you please pull it and check locally? |
@Khady i want to debug the failing builds but running https://gist.github.com/Athishpranav2003/0dce8e130add559644135913220f1ef3 |
@rgrinberg could you please help me with the make issue? |
I think that you need to install the test dependencies
I don't know if strace is the binary to trace system calls. If it is then it should be installed with a package manager on linux. Same for the node binary (nodejs). |
@Khady other errors are resolved except |
This one would be handled by the Line 58 in d9b0e2e
|
I have it installed but still the error persists |
Is it still the same error as you shared before? This is what I have installed
|
|
@Khady did you try running the PR locally? Was it working ? |
Looks like there is still a problem. The execution with an absolute path only works if it was done with a relative path first.
The other tests looked fine, but I didn't spend too much time trying. |
@Khady so basically it needs to build before run and the build option was not present before in abs path(i also followed same to preserve behaviour but will change it now). I can add that as well now and push it. Give me an hour |
Signed-off-by: Athish Pranav D <[email protected]>
ed1866f
to
b3eeb4f
Compare
@Khady now i have changed the program to run build even when passing absolute path. |
Addressing #10536