fix: add tolerant tar extraction on windows to solve failing on symlinks #1138
Conversation
|
Thank you! On first glance, looks good! |
|
@wolfv i think I've fixed the linting issue. Let me know if there is anything else I should consider. |
| match file.header().entry_type() { | ||
| EntryType::Symlink if cfg!(target_os = "windows") => { | ||
| if let Err(e) = file.unpack(&path) { | ||
| println!( |
There was a problem hiding this comment.
We should always use tracing::warn! or tracing::error!.
| } | ||
| } | ||
| Err(e) => { | ||
| println!("Warning: failed to read an entry due to {:?}", e); |
There was a problem hiding this comment.
I think in this case, we should also return the error and fail the operation.
| } | ||
| _ => { | ||
| if let Err(e) = file.unpack(&path) { | ||
| println!("Warning: failed to unpack {:?} due to {:?}", path, e); |
There was a problem hiding this comment.
Again we should return the error here :)
|
Hi @Krande - let me know if you want to make the changes or if you would prefer me taking over :) |
|
If you have time to implement the changes, feel free to finish them:) if not, I'll likely start to implement them after lunch time tomorrow! |
|
I looked a little bit deeper and I think unfortunately this doesn't cover a bunch of (other) edge cases that the original tar extraction code covers. That's really unfortunate. Ideally we could implement this kind of behavior / filtering in tar-rs directly. |
|
I see. I checked tar-rs and found this alexcrichton/tar-rs#280 which I guess is related. |
|
Would it be possible to make this tolerant extraction feature optional by hiding it behind an opt-in flag until a better long-term solution can be found? That way, it is at least possible for us forced to use windows in our day jobs to use rattler-build for projects containing symlinks :) |
|
A "shitty" workaround is to keep the source as tarball. This is possible since the last release or two by adding a option: Then you can add e.g. Is that an OKish workaround? Sorry for not coming up with something better. I would love an option in the tar crate. Maybe you could also work on a PR over there? |
Yeah, anything that works is fine for now :) That at least gives me a chance at using rattler-build for more packages :)
Sure, I can try. It would also help if you can shed some light on the types of edge cases a filtering behaviour in tar-rs should be able to cover? Referring to your comment
|
|
@Krande I think if you add this behavior to the original tar, then we can keep using the existing You could add a SymlinkBehavior enum to tar-rs and have Fail, Warn or Copy options and then implement them accordingly when symlinking fails. |
Hey guys,
this is supposed to close #940.
This is my first time writing rust beyond just "hello world", so any suggestions for improvements are most welcome!