-
Notifications
You must be signed in to change notification settings - Fork 5
Spoorcc/issue822: Create cross-platform installers to make it less virus scanner scary #861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds cross‑platform packaging: CI builds standalone Nuitka binaries, a new Changes
Sequence DiagramsequenceDiagram
autonumber
participant Dev as Developer
participant GH as GitHub Actions
participant Build as Nuitka Builder
participant Pack as Packaging Script
participant Art as Artifacts
Dev->>GH: push / tag → trigger workflow
GH->>Build: build standalone binaries (Nuitka) -> BUILD_DIR
GH->>Pack: run `python script/package.py` with BUILD_DIR
alt Linux
Pack->>Pack: fpm -> .deb / .rpm
else macOS
Pack->>Pack: fpm -> .pkg
else Windows
Pack->>Pack: generate WiX .wxs/.wixproj
Pack->>Pack: dotnet / WiX -> .msi
end
Pack->>Art: upload installers (.deb, .rpm, .pkg, .msi)
Art-->>Dev: artifacts available
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ 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 |
b57cd4f to
7e2377b
Compare
7e2377b to
5fefe62
Compare
5fefe62 to
6339be5
Compare
6339be5 to
584366b
Compare
584366b to
82e8edd
Compare
82e8edd to
fb9176f
Compare
fb9176f to
3d2d7c5
Compare
3d2d7c5 to
c32b322
Compare
c32b322 to
a722518
Compare
a722518 to
4253f14
Compare
4253f14 to
e3649b7
Compare
e3649b7 to
9e60a08
Compare
9e60a08 to
08c391f
Compare
b21f27d to
97c5046
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.devcontainer/Dockerfile (1)
12-23: Consider updating Ruby version (matches CI concern).This installation uses Ruby 2.7, which reached end-of-life in March 2023. While it matches the CI workflow for consistency, consider updating to a supported Ruby version (3.x) across both the devcontainer and CI to receive security updates.
This is the same concern raised for the CI workflow configuration.
.github/workflows/build.yml (1)
37-47: Consider updating Ruby version to a currently supported version.Ruby 2.7 reached end-of-life on March 31, 2023. Update to Ruby 3.x (latest: 3.4.8) to receive security updates and bug fixes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.devcontainer/Dockerfile.github/workflows/build.ymlREADME.mddfetch/resources/__init__.pypyproject.tomlscript/package.py
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~58-~58: The operating system from Apple is written “macOS”.
Context: ... in the run. - Linux .deb & .rpm - MacOS .pkg - Windows .msi ## Github Acti...
(MAC_OS)
⏰ 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). (16)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (ubuntu-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: build (macos-latest)
- GitHub Check: DevContainer Build & Test
- GitHub Check: test-cygwin
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: test
🔇 Additional comments (16)
pyproject.toml (3)
193-193: LGTM: Standalone mode appropriate for packaging.The switch from
onefiletostandalonemode aligns with the PR objective to create proper OS installers. Standalone distributions are generally better received by virus scanners and allow for more natural packaging into deb/rpm/pkg/msi formats.
206-208: LGTM: Simplified output filenames.Removing version placeholders from the binary filenames is appropriate since version information is now managed by the OS-specific package installers (deb, rpm, pkg, msi). This simplifies the build output and aligns with standard packaging practices.
199-201: No action needed—package configuration is correctly aligned.The
dfetchpackage is implicitly included via the entry point (dfetch = "dfetch.__main__:main"), which ensures Nuitka compiles the entire package by tracing imports fromdfetch.__main__. The resource mappinginclude-data-dir="dfetch/resources=resources"correctly aligns with the resource resolution logic indfetch/resources/__init__.py(line 14), which looks for a"resources"string when compiled. Theinclude-package-data="infer_license"directive properly handles the external dependency..github/workflows/build.yml (6)
49-57: LGTM: WiX tooling setup is sound.The Windows-specific setup properly installs WiX 6.0.2 and configures the PATH. The version verification step is a good practice to catch installation issues early.
84-84: LGTM: Packaging step placement is correct.The
python script/package.pyinvocation is appropriately placed after the binary build and will utilize the OS-specific tooling installed in earlier steps.
90-94: LGTM: Explicit artifact collection improves clarity.The updated artifact collection explicitly lists each package format (.deb, .rpm, .pkg, .msi) rather than using a glob pattern. This makes it clearer what artifacts are expected and helps catch packaging issues early.
120-124: Note the macOS path quirk (expected behavior).The PATH addition
/opt/dfetch/opt/dfetchappears to have a duplicate segment, but this is due to a known fpm issue (#1996) as documented inscript/package.pyline 83. The double prefix is expected behavior when using fpm for macOS packages.
127-137: LGTM: Windows MSI installation with good logging.The MSI installation approach with concurrent log tailing is well-structured for CI debugging. The process management correctly handles the installer and log viewer.
Minor note:
$procLog.Kill()might throw an exception if the process has already exited, but PowerShell will continue and the step will still succeed.
139-145: LGTM: Binary execution via PATH validates installation.The updated commands now execute
dfetchdirectly from PATH rather than./dfetch, which properly validates that the OS-specific installers correctly configured the system PATH.script/package.py (6)
1-23: LGTM: Configuration and imports are well-structured.The script imports are appropriate, and the packaging metadata (name, description, maintainer, URL, license) is properly defined. The use of
/opt/dfetchas the installation prefix follows standard conventions for third-party software on Unix-like systems.
36-66: LGTM: Linux packaging logic is sound.The function correctly builds both
.deband.rpmpackages using fpm with appropriate metadata and installation prefixes. The approach of changing directory with-Cand packaging all files with.is standard practice.
69-98: LGTM: macOS packaging with documented workaround.The macOS packaging correctly uses fpm with the
osxpkgtarget. The comment on line 83 properly documents the known fpm issue #1996 that causes the double prefix path (/opt/dfetch/opt/dfetch), which aligns with the PATH configuration in the CI workflow.
190-198: LGTM: WiX project file generation is correct.The project file structure is appropriate for WiX Toolset v4 SDK-style builds and correctly references the WiX source file.
201-217: LGTM: Windows packaging workflow is well-structured.The function correctly orchestrates the WiX-based MSI generation: creates output directory, generates required WiX files, verifies tooling, and builds the installer using the WiX v4 SDK approach.
220-248: LGTM: Main orchestration logic is sound.The
main()function properly validates prerequisites, provides helpful output vialist_files(), and correctly dispatches to platform-specific packaging functions. Error handling for missing build directory and unsupported platforms is appropriate..devcontainer/Dockerfile (1)
38-38: LGTM: pip version update.The pip version bump from 25.2 to 25.3 is routine maintenance and should not introduce any issues.
97c5046 to
1e0720d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.devcontainer/Dockerfile.github/workflows/build.ymlREADME.mddfetch/resources/__init__.pypyproject.tomlscript/package.py
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- script/package.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-25T22:46:21.776Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 861
File: dfetch/resources/__init__.py:12-17
Timestamp: 2025-12-25T22:46:21.776Z
Learning: In dfetch, when building standalone binaries with Nuitka, resources are packaged at the top level as "resources" (not "dfetch.resources") to avoid name clashes with the binary executable named "dfetch". The code in dfetch/resources/__init__.py uses `"resources"` string when `__compiled__` is detected, which correctly references the top-level resources folder in the standalone build.
Applied to files:
pyproject.toml
📚 Learning: 2025-12-25T22:46:21.776Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 861
File: dfetch/resources/__init__.py:12-17
Timestamp: 2025-12-25T22:46:21.776Z
Learning: In Nuitka standalone builds for dfetch, ensure resources are referenced from the top-level 'resources' directory (not the package path 'dfetch.resources') to avoid clashes with the executable named 'dfetch'. Specifically, in code under dfetch/resources/, when __compiled__ is detected, use the top-level 'resources' path as the resource locator (as already done in dfetch/resources/__init__.py with the 'resources' string). This pattern should apply to all Py files within dfetch/resources/ to prevent name collisions in standalone binaries.
Applied to files:
dfetch/resources/__init__.py
⏰ 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). (17)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (macos-latest, 3.14)
- GitHub Check: test (macos-latest, 3.12)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: test
- GitHub Check: DevContainer Build & Test
- GitHub Check: test-cygwin
🔇 Additional comments (12)
dfetch/resources/__init__.py (1)
12-17: LGTM! Resource path resolution correctly handles compiled and non-compiled builds.The conditional logic properly addresses the standalone binary structure where resources are at the top level to avoid name clashes with the
dfetchexecutable. This aligns with theinclude-data-dir="dfetch/resources=resources"mapping in pyproject.toml.Based on learnings, this implementation correctly uses the "resources" string for compiled builds and the resources module for development/package installs.
pyproject.toml (3)
193-193: LGTM! Standalone mode aligns with the cross-platform packaging strategy.The switch from "onefile" to "standalone" mode creates a distributable directory structure that's more suitable for packaging into .deb, .rpm, .pkg, and .msi installers.
199-201: LGTM! Data inclusion configuration supports the standalone build structure.The
include-data-dirmapping correctly places resources at the top level of the standalone build, and theinclude-package-dataandinclude-moduledirectives ensure theinfer_licensepackage and its data are included.Based on learnings, this configuration harmonizes with the resource path resolution in dfetch/resources/init.py.
206-208: LGTM! Simplified output filenames improve packaging workflow.Removing version placeholders from the binary names makes them easier to reference in packaging scripts and CI workflows.
.devcontainer/Dockerfile (2)
38-38: LGTM! Pip upgrade to 25.3 is appropriate.Keeping pip up to date ensures access to the latest bug fixes and features.
12-23: Ruby 2.7 EOL concern is valid, but upgrading is not feasible on Debian 11.While Ruby 2.7 reached end-of-life in March 2023 and FPM 1.17.0 does support Ruby 3.x, upgrading Ruby requires Debian 12+ as Ruby 3.x is not available in the Debian 11 (Bullseye) main archive. To upgrade, you would need to either upgrade the base OS or use external Ruby providers. Since this is only for development/packaging, deferring this until base OS upgrades is reasonable.
Likely an incorrect or invalid review comment.
.github/workflows/build.yml (6)
84-84: LGTM! Packaging script invocation follows the build step.The packaging script is correctly invoked after the binary build to create platform-specific installers.
90-94: LGTM! Artifact collection covers all target package formats.The patterns correctly capture Linux (.deb, .rpm), macOS (.pkg), and Windows (.msi) installers from the packaging output directory.
127-137: MSI installation process looks thorough.The Windows installation correctly:
- Installs silently with logging
- Captures logs in real-time
- Waits for completion
- Updates PATH
The concurrent log tailing process is a nice touch for debugging.
139-145: LGTM! Binary testing uses the installed command.The test commands correctly use
dfetchdirectly (not./dfetch), which verifies that the PATH updates and binary installations work correctly across all platforms.
112-125: The nested macOS PATH is intentional. The/opt/dfetch/opt/dfetchstructure is documented inscript/package.py(line 86) as a known behavior of fpm when using the--prefixflag, with a reference to fpm issue #1996. No changes needed.Likely an incorrect or invalid review comment.
49-57: WiX 6.0.2 is confirmed stable (servicing release, 28 Aug 2025). If this project is migrating from WiX 3.x, address documented breaking changes: schema/namespace updates, MSBuild integration, component GUID stability, bootstrapper/bundle changes, and extension API updates. The installation script itself is appropriate for v6.0.2.
1e0720d to
78d0c20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.devcontainer/Dockerfile.github/workflows/build.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .devcontainer/Dockerfile
⏰ 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). (18)
- GitHub Check: Publish Python distribution 📦 to TestPyPI
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (ubuntu-latest, 3.11)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: DevContainer Build & Test
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: test-cygwin
- GitHub Check: test
🔇 Additional comments (6)
.github/workflows/build.yml (6)
37-47: LGTM! Ruby 3.3 and fpm setup is correct.The upgrade to Ruby 3.3 addresses the previous EOL concern with Ruby 2.7, and pinning fpm to 1.17.0 ensures reproducible builds. The platform gating for non-Windows runners is appropriate given Windows uses WiX tooling instead.
49-57: LGTM! Windows packaging tooling setup is correct.The MSBuild and WiX installation steps are properly gated to Windows runners, use appropriate version pinning (WiX 6.0.2), and include version verification. The PATH configuration correctly uses PowerShell syntax for Windows.
84-84: LGTM! Packaging step correctly invokes the packaging script.The addition of the packaging step after the build is appropriate and will generate the platform-specific installers.
90-94: LGTM! Artifact collection patterns are more specific.The updated paths explicitly collect platform-specific package formats (.deb, .rpm, .pkg, .msi) from the build/dfetch-package directory. GitHub Actions will handle cases where a platform doesn't produce all formats gracefully.
112-118: LGTM! Ubuntu installation uses proper package management.The switch from symlinks to
dpkg -iis the correct approach for testing the actual .deb package. The file listing withdpkg -Lprovides useful debugging information, and the PATH configuration looks correct.
139-145: LGTM! dfetch commands correctly use the installed binary.The switch from
./dfetchtodfetchis appropriate for testing the installed package. The test suite covers key functionality (init, environment, validate, check, update, report), and runningupdatetwice likely tests idempotency.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.