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
| 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.
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.
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 <athishanna@gmail.com>
Signed-off-by: Athish Pranav D <athishanna@gmail.com>
Signed-off-by: Athish Pranav D <athishanna@gmail.com>
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 <athishanna@gmail.com>
ed1866f to
b3eeb4f
Compare
|
@Khady now i have changed the program to run build even when passing absolute path. |
|
I still have the same behavior as in #10857 (comment) |
|
(I'm not sure what's the best way to do it, but it would be worth having a test to ensure the right behavior remains) |
|
@Khady so i believe the structure of dune is itself kinda very different from what we are trying to do, I guess some small things is still left but i dont think we can achieve the full functionality without larger diff |
|
Yes I do agree that the fix should probably be deeper. Then it would also cover other commands where the issue is the same. Like |
|
@Alizter does the PR you merged replace this PR? Should we close this PR? |
|
Yes, #12094 should subsume this. |
Addressing #10536