Fixes tmpdir & AppImage writing issues #741
Conversation
caesay
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR! There are a few things that can be simplified, if you don't get around to it I can fix and merge later.
| // Ensure the copied runtime is writable before appending squashfs. | ||
| try { | ||
| if (VelopackRuntimeInfo.IsLinux) { | ||
| #if NET8_0_OR_GREATER |
There was a problem hiding this comment.
I don't think any of this code that calls SetUnixFileMode is necessary. ChmodFileAsExecutable already sets 755, is that not sufficient?
| tempDir = "/tmp/velopack"; | ||
| } else if (VelopackRuntimeInfo.IsWindows) { | ||
| tempDir = Path.Combine(Path.GetTempPath(), "Velopack"); | ||
| var velopackTemp = Environment.GetEnvironmentVariable("VELOPACK_TEMP"); |
There was a problem hiding this comment.
Is there any reason why VELOPACK_TEMP isn't treated the same as the other environment variables? We could simplify this code significantly:
var envTempDir = new[] { "VELOPACK_TEMP", "TMPDIR", "TEMP", "TMP" }
.Select(Environment.GetEnvironmentVariable)
.FirstOrDefault(x => x != null);
|
|
||
| if (Options.PackDirectory.EndsWith(".AppDir", StringComparison.OrdinalIgnoreCase)) { | ||
| var packDirName = Options.PackDirectory?.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); | ||
| var packDirBase = string.IsNullOrWhiteSpace(packDirName) ? Options.PackDirectory : packDirName; |
There was a problem hiding this comment.
Not sure I understand this line of code. If the goal is to allow trailing slashes then the TrimEnd would've been the only thing we needed to add and I think we have a guarantee here already that Options.PackDirectory is not null.
There was a problem hiding this comment.
--packDir is sensitive to path-to/foo.AppDir/ and vpk will only operate if the parameter is path-to/foo.AppDir, however using tab autocomplete the trailing / is added, requiring to manually edit the command.
Will use TrimEnd.
This should fix #738 and #739