pop-os / system76-firmware

System76 Firmware Tool and Daemon
GNU General Public License v3.0
75 stars 28 forks source link

Fix argument order for `with_prefix_in` #122

Closed crawfxrd closed 1 year ago

crawfxrd commented 1 year ago

The argument order is wrong, causing upgrades to fail when moving the temp extract to the EFI partition.

system76-firmware: failed to schedule: failed to move /boot/efiMG6RBk to /efi/boot/system76-firmware-update: Invalid cross-device link (os error 18)

Additionally, add a . as a separator to the prefix to exactly match the old tempdir behavior, as tempfile does not add a separator itself.

Fixes: 4613ff0badba ("Update edition, deps, toolchain") Ref: tempfile::TempDir#with_prefix_in Resolves: #120

crawfxrd commented 1 year ago

While at it, should check exact name behavior for tempdir vs tempfile. I think tempdir might have added a separator (.) between the prefix and random ID, but tempfile requires specifying the separator (as shown in the docs).

This doesn't break behavior, but would make it exactly match the old behavior.

Yep: https://github.com/rust-lang-deprecated/tempdir/blob/403f918d9f3708ca144c727d20d2c1f8738767fc/src/lib.rs#L204

orangecms commented 1 year ago

The new function's docs also say:

The directory and everything inside it will be automatically deleted once the returned TempDir is destroyed.

I.e., the cleanup in the error handling can also be removed then?

crawfxrd commented 1 year ago

That's always been the case (tempdir had the same behavior), but it's outside the scope of the cleanup I did.

orangecms commented 1 year ago

Awesome, thanks y'all for the quick responses and fixes! 🧡