sonos / dinghy

Easier cross-compilation for phones and single boards computers
Other
367 stars 44 forks source link

On the use `std::fs::ReadDir.next()` #177

Closed n-eq closed 2 years ago

n-eq commented 2 years ago

Hello,

This is more of an open question but I was wondering about the use of std::fs::ReadDir.next() to determine the paths to Android tools binaries.

In my case, it turned out that running a test failed on my Android phone, throwing the following error: [2022-08-08T07:56:49Z ERROR cargo_dinghy] Not a directory (os error 20).

Further investigation led me to this line: https://github.com/sonos/dinghy/blob/14edaadfaf833dd21a8daba528b8cabe0807059a/dinghy-lib/src/android/mod.rs#L61

And it turns out the error comes from the fact that next() took .DS_Store directory instead of the expected bin. I noticed the same pattern is used in other parts of the project (at least in the same file referenced above).

Although this is not blocking behavior - I managed to figure out quickly and fix it up -, I was wondering if there are good reasons to use next() in this context instead of trying to find the path that is expected, which, in this case, would probably be possible with a combination of joins and exists.

Thanks! Nabil

kali commented 2 years ago

Hey, thanks for your interest in Dinghy, and sorry for this issue.

I think the reason why we used this trick is to avoid making an assumption on the next token in the path: it's the build host triple, so it is variable across platforms. Not super hard to deal with, but I guess we got lazy.

I'm not sure if we want to hard-code it. With the Apple platforms in-between two architectures at this time, we may very well have one or the other on Apple silicon. Another approach could be to iterate till we find a directory...

fredszaq commented 2 years ago

The name of the folder is indeed dependent on the host system and is different on linux windows and macOS, I made a fix ensuring we only look at directories in this case.

fredszaq commented 2 years ago

fix released in 0.6.2