Open JanCVanB opened 2 years ago
@Anton-4 @ayazhafiz What chaos have we wrought?!
I am gonna try and bisect
file.roc has been broken for a quite a while:
21b74f6dbbe8fe3dc309dd2cb0c94fd6d5b04a65 is the first bad commit
commit 21b74f6dbbe8fe3dc309dd2cb0c94fd6d5b04a65
Author: Richard Feldman <oss@rtfeldman.com>
Date: Mon Sep 12 16:12:36 2022 -0400
Add Dir and Env to CLI platform
Hmmm, that looks wrong, I definitely ran it successfully last week... any chance the root cause is bleeding across bisection steps?
I used the newest version of the nix environment the entire time, so it could be related to rust versioning changes that only now made an issue present. Or something else of that nature.
args.roc has also been broken for quite a while:
c9e73b67617818223d60a52a84df4ecb9efd97dc is the first bad commit
commit c9e73b67617818223d60a52a84df4ecb9efd97dc
Author: Ayaz Hafiz <ayaz@rwx.com>
Date: Wed Sep 14 12:06:35 2022 -0500
Change example to be a calculator
Anyway, too tired too investigate this more now. At least we now have some specific CLs to investigate.
Thank you, @bhansconnect !
I think this is platform-specific, these work just fine on latest main on an M1.
We have test coverage for "file" and "args" so I'm unsure why commits that far back would be seen as broken now.
I agree. I might have time to bisect today, and I just found a good (very old, from Sep 12) commit on my machine:
[jan@framey roc]$ git checkout bc13454
Previous HEAD position was 6a9a1330b Merge pull request #4035 from roc-lang/windows-linker-preprocess-exe
HEAD is now at bc13454d1 Merge pull request #4016 from roc-lang/dependabot/cargo/pretty_assertions-1.3.0
[jan@framey roc]$ nix develop
[jan@framey roc]$ roc dev examples/interactive/file.roc
🔨 Rebuilding platform...
Writing a string to out.txt
Successfully wrote a string to out.txt
[jan@framey roc]$
My bisect on file.roc
health agrees with @bhansconnect (21b74f6), but that still doesn't match my memory...
Oh, I know why it doesn't match my memory - because my memory is probably of success on my mac! Maybe it's always been broken on Linux...
For file.roc
the issue looks to be a glue issue, but it is hard to tell for sure. Maybe with the last update CL, glue didn't get updated but it should have been. As such, we are freeing something that doesn't really exist or something of that nature. Not fully sure, because I get different errors if I build and run in nix vs if I build and run outside of nix. Another possibility is that it is a Rust bug related to freeing DirEntry's. But I think that error is a mis-attribution by GDB.
Ok, I am exceptionally confused:
I rewrote the body of roc_fx_dirList
since it was crashing. The new body is this:
let mut out = RocList::empty();
match std::fs::read_dir(path_from_roc_path(roc_path)) {
Ok(dir_entries) => {
for result in dir_entries {
match result {
Ok(entry) => {
let roc_path = dbg!(os_str_to_roc_path(dbg!(entry.path().into_os_string().as_os_str())));
// out.extend_from_slice(&[roc_path]);
}
Err(e) => todo!("handle errors: {}", e)
}
}
RocResult::ok(out)
}
Err(_) => {
todo!("handle Dir.list error");
}
}
This version returns an empty list, but does successfully convert all paths to bytes and print them out with dbg!
. If you remove the comment on out.extend_from_slice(&[roc_path]);
it will crash. The crash does not happen in out.extend_from_slice(&[roc_path]);
. Instead, after a few iterations, the result from dir_entries
is an error. So somehow, filling the roc list leads to breaking the result of a later iteration of the for loop.
Ok, yeah, very not sure why it is broken, but I figured out a minimal diff to fix. Simply collect in a vector and then convert to a RocList. Still really confused why this cause the dir_entries
to break rather than the RocList, but I guess we are probably overwriting memory we don't own and breaking the dir_entries
. So probably a RocList
in rust bug.
--- a/examples/cli/cli-platform/src/lib.rs
+++ b/examples/cli/cli-platform/src/lib.rs
@@ -407,16 +407,17 @@ pub extern "C" fn roc_fx_dirList(
) -> RocResult<RocList<RocList<u8>>, WriteErr> {
println!("Dir.list...");
match std::fs::read_dir(path_from_roc_path(roc_path)) {
- Ok(dir_entries) => RocResult::ok(
- dir_entries
+ Ok(dir_entries) => {
+ let vec = dir_entries
.map(|opt_dir_entry| match opt_dir_entry {
Ok(entry) => os_str_to_roc_path(entry.path().into_os_string().as_os_str()),
Err(_) => {
todo!("handle dir_entry path didn't resolve")
}
})
- .collect::<RocList<RocList<u8>>>(),
- ),
+ .collect::<Vec<RocList<u8>>>();
+ RocResult::ok(RocList::from(&vec[..]))
+ }
Err(_) => {
todo!("handle Dir.list error");
}
For args.roc, it is the know surgical linker bug #3609. Just made a PR to at least print a better error message when this comes up.
We have test coverage for "file" and "args" so I'm unsure why commits that far back would be seen as broken now.
For args.roc, it is the know surgical linker bug #3609. Just made a PR to at least print a better error message when this comes up.
Just a note; in the cli_run
tests the args
example is tested with the legacy linker, which is why it does not fail on CI.
We have test coverage for "file" and "args" so I'm unsure why commits that far back would be seen as broken now.
There is/was no test coverage for file.roc
. I think that's because it used to be under the interactive folder which was exempt from needing to have tests.
Recent PRs modifying examples have seemed to break some stuff: