Preserve unix file mode for files copied into package zips #109
Conversation
WalkthroughThis pull request upgrades the target framework from net6.0/net7.0 to net8.0 across both ThunderstoreCLI and ThunderstoreCLI.Tests projects. In BuildCommand.cs, a new polymorphic Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #109 +/- ##
=========================================
- Coverage 5.47% 5.46% -0.01%
=========================================
Files 42 41 -1
Lines 2413 2416 +3
Branches 238 238
=========================================
Hits 132 132
- Misses 2278 2281 +3
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ThunderstoreCLI.Tests/ThunderstoreCLI.Tests.csproj (1)
11-27: Upgrade test tooling packages to support .NET 8The test SDK and collector packages are significantly outdated. Update to latest stable versions:
- Microsoft.NET.Test.Sdk: 17.4.1 → 18.0.0
- xunit: 2.4.2 → 2.9.3
- xunit.runner.visualstudio: 2.4.5 → 3.1.5
- coverlet.collector: 3.2.0 → 6.0.4
🧹 Nitpick comments (6)
ThunderstoreCLI/Commands/BuildCommand.cs (2)
197-201: Tighten manifest default perms to 06440644 is a safer default for text files than 0664.
- plan.AddPlan("manifest.json", new EntryData.FromMemory(Encoding.UTF8.GetBytes(SerializeManifest(config)), (UnixFileMode)0b110_110_100)); // rw-rw-r-- + plan.AddPlan("manifest.json", new EntryData.FromMemory(Encoding.UTF8.GetBytes(SerializeManifest(config)), (UnixFileMode)0b110_100_100)); // rw-r--r--
220-239: Add a regression test for exec-bit preservationTo lock the fix, add a test that:
- Creates a temp file, sets 0755, builds a zip entry, then asserts ((entry.ExternalAttributes >> 16) & 0x1FF) == 0o755.
I can draft a small xUnit test that runs on Linux/macOS CI and skips on Windows if needed.
ThunderstoreCLI.Tests/ThunderstoreCLI.Tests.csproj (2)
6-6: Align C# language version with .NET 8Pinning LangVersion to 11 limits features and analyzers on net8. Consider removing it (use SDK default C# 12) or set to 12.
- <LangVersion>11</LangVersion> + <!-- Prefer SDK default for net8 (C# 12) --> + <LangVersion>12</LangVersion>
13-16: Remove PolySharp in tests on net8PolySharp is unnecessary on .NET 8; keeping it adds build time and potential analyzer noise.
- <PackageReference Include="PolySharp" Version="1.12.1"> - <PrivateAssets>all</PrivateAssets> - <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> - </PackageReference>ThunderstoreCLI/ThunderstoreCLI.csproj (2)
6-6: Use C# 12 (or remove explicit LangVersion)For net8, prefer SDK default (C# 12). Keeping 11 may restrict newer compiler/analyzers.
- <LangVersion>11</LangVersion> + <LangVersion>12</LangVersion>
36-41: Drop PolySharp on .NET 8The comment indicates it was needed for NET 6. On net8 it’s redundant.
- <!-- needed to use some C#11 features with NET 6, like required properties. --> - <PackageReference Include="PolySharp" Version="1.12.1"> - <PrivateAssets>all</PrivateAssets> - <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> - </PackageReference>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ThunderstoreCLI.Tests/ThunderstoreCLI.Tests.csproj(1 hunks)ThunderstoreCLI/Commands/BuildCommand.cs(11 hunks)ThunderstoreCLI/ThunderstoreCLI.csproj(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ThunderstoreCLI/Commands/BuildCommand.cs (3)
ThunderstoreCLI/Configuration/Config.cs (4)
Config(8-164)Config(28-32)Config(33-42)Config(115-143)ThunderstoreCLI/Utils/Write.cs (2)
Write(5-89)Error(24-27)ThunderstoreCLI/Program.cs (2)
CommandException(83-86)CommandException(85-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test macOS-latest
- GitHub Check: Build and test windows-latest
- GitHub Check: Build and test ubuntu-latest
🔇 Additional comments (1)
ThunderstoreCLI/Commands/BuildCommand.cs (1)
230-239: I'll verify the core concerns in this review by examining the codebase context and the implementation details.Now let me verify the specifics in the codebase:
Now let me verify the actual code in the repository to understand what
entry.Value.GetUnixFileMode()returns:Based on my research, I have verified the review comment against relevant documentation and real-world issues in the .NET ecosystem:
Summary of findings:
The ZIP file format uses high 16 bits of external file attributes to store Unix file mode, including file-type bits (e.g., 0100000 for regular files) in the top 4 bits and permissions in the lower bits.
The .NET runtime had a regression issue (#76006) where ZIP archive entries were missing file-type bits in external attributes, causing files to be inaccessible when extracted on macOS.
File.GetUnixFileMode() returns only the UnixFileMode enum, which represents filesystem permissions without file-type bits. This means the current code setting
entry.Value.GetUnixFileMode() << 16captures permissions but omits the S_IFREG (0x8000) file-type bit.For Unix systems, the high order byte of ExternalAttributes is frequently used to store file permissions, and the standard expects both file-type and permission bits.
The review comment's core concern is valid: the code should include the S_IFREG file-type bit alongside permission bits. Both suggested options (always set, or keep the Windows gate but add the type bit) are defensible approaches to fix this. The Windows gate rationale is conservative but not unreasonable—however, many tools honor the upper 16 bits regardless of build platform, so Option A (always set) is more robust for cross-platform compatibility.
| foreach (string filename in Directory.EnumerateFiles(basePath, "*.*", SearchOption.AllDirectories)) | ||
| { | ||
| var targetPath = FormatArchivePath($"{destDirectory}{filename[(basePath.Length + 1)..]}"); | ||
| plan.AddPlan(targetPath, () => File.ReadAllBytes(filename)); | ||
| plan.AddPlan(targetPath, new EntryData.FromFile(filename)); | ||
| } |
There was a problem hiding this comment.
Fix: directory file search skips files without extensions
Using "." will miss files like "script", "LICENSE", etc. Use "*" to include all files.
Apply:
- foreach (string filename in Directory.EnumerateFiles(basePath, "*.*", SearchOption.AllDirectories))
+ foreach (string filename in Directory.EnumerateFiles(basePath, "*", SearchOption.AllDirectories))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach (string filename in Directory.EnumerateFiles(basePath, "*.*", SearchOption.AllDirectories)) | |
| { | |
| var targetPath = FormatArchivePath($"{destDirectory}{filename[(basePath.Length + 1)..]}"); | |
| plan.AddPlan(targetPath, () => File.ReadAllBytes(filename)); | |
| plan.AddPlan(targetPath, new EntryData.FromFile(filename)); | |
| } | |
| foreach (string filename in Directory.EnumerateFiles(basePath, "*", SearchOption.AllDirectories)) | |
| { | |
| var targetPath = FormatArchivePath($"{destDirectory}{filename[(basePath.Length + 1)..]}"); | |
| plan.AddPlan(targetPath, new EntryData.FromFile(filename)); | |
| } |
🤖 Prompt for AI Agents
In ThunderstoreCLI/Commands/BuildCommand.cs around lines 266 to 270, the
Directory.EnumerateFiles call uses the search pattern "*.*" which skips files
without extensions (e.g., LICENSE, script); change the pattern to "*" so all
files are included, keep SearchOption.AllDirectories and the rest of the loop
logic the same, and ensure the targetPath computation and plan.AddPlan call
remain unchanged.
This PR makes it so Unix file mode is preserved into the resulting archive. As a result of how I rewrote the planner, it also now avoids bringing entire files into memory at a time, which should avoid issues around files being too large to fit into memory. Fixes #107.
Also updates to .NET 8.0 since net6/net7 are out of support now.