Use relative paths in generated erlang -file attributes #4193
Conversation
oneness
changed the title
that it needs to handle Windows as well.
compiler-core/src/erlang.rs
Outdated
| .unwrap_or_else(|| { | ||
| EcoString::from( | ||
| src_path_full | ||
| .strip_prefix(&(root_str + "/")) |
There was a problem hiding this comment.
Is this going to work on Windows?
Might be more reliable to use the path methods to make a relative path rather than do it via string manipulation.
There was a problem hiding this comment.
My first commit here:
d5b096a
failed on windows test. It used platform independent path.
After the test failure, I realized that all of the erlang target snapshot tests are assuming unix path and we need to keep two version of each (correct me if I am wrong) snaps file.
I agree with using path abstraction to handle this is much better than string manipulation but I am afraid this branch might sprawl into a long running with more code/test changes considering we have few places where we directly use "/" in our code (manifest.rs memory.rs and few places in test codes).
So I made a note of myself to do a bug report to fix this to work in Windows. What do you think?
There was a problem hiding this comment.
we need to keep two version of each (correct me if I am wrong) snaps file.
We edit file paths to be unix style for snapshots any time there would be difference between platforms.
I don't think there would need to be any here though, no? The snapshots all use unix style path roots already.
So I made a note of myself to do a bug report to fix this to work in Windows. What do you think?
We don't accept contributions that don't work on all our supported platforms I'm afraid.
There was a problem hiding this comment.
Thanks for the feedback @lpil.
Yes, we do normalize windows path to be unix style for snapshots during snap file generation. Since all snapshots use unix style path roots, turning the directive absolute path (which happen to be os dependent during run time) into relative is run time dependent as well. if you are ok with following:
For Unix (note the ./ before the src dir) :
Current working directory: /root/project
Source file location: /root/project/src/wibble.gleam
-file value: ./src/wibble.gleam
For Windows (Note .\ before src dir):
Current working directory: \root\project
Source file location: \root\project\src\wibble.gleam
-file value: .\src\wibble.gleam
Then we can drop the hard coded "/", instead just prepend ".", which I believe works cross platform. What do you think?
There was a problem hiding this comment.
That behaviour looks correct to me, aye!
compiler-core/src/erlang.rs
Outdated
| src_path_full | ||
| .strip_prefix(&root_str) | ||
| .map(|remaining| format!(".{}", remaining)) | ||
| .unwrap_or_else(|| src_path_full.to_string()), |
There was a problem hiding this comment.
Do this using path manipulation please, not string manipulation.
Original issues #4141 and per @lpil, the expected behaviour should be:
Current working directory: /root/project
Source file location: /root/project/src/wibble.gleam
-file value: src/wibble.gleam