Improve binary icon's & file information #898
Conversation
📝 WalkthroughWalkthroughAdds Nuitka packaging options and icon/product metadata to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / Build
participant Config as pyproject.toml (Nuitka)
participant Script as script/package.py
participant WiX as WiX Generator
participant MSI as MSI Builder
Dev->>Config: commit Nuitka icon/product/data fields
Dev->>Script: invoke packaging script
Script->>Config: read ICON, product metadata, URLs
Script-->>Script: set WINDOWS_ICO_PATH, ARP properties (comments, URLs, hide modify/repair)
Script->>WiX: emit WiX XML with AppIcon + ARP properties
WiX->>MSI: include icon and ARP metadata into installer
MSI-->>Dev: produce installer (.msi / binaries)
note right of WiX `#e6f7ff`: New/changed interactions\nhighlighted: icon & ARP metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
doc/images/dfetch_logo_only.pngis excluded by!**/*.pngdoc/images/dfetch_logo_only.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T22:46:27.554Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 861
File: dfetch/resources/__init__.py:12-17
Timestamp: 2025-12-25T22:46:27.554Z
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
⏰ 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 (windows-latest, 3.11)
- GitHub Check: test (ubuntu-latest, 3.9)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (macos-latest, 3.12)
- GitHub Check: test (macos-latest, 3.9)
- GitHub Check: test (ubuntu-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test-cygwin
- GitHub Check: test
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (2)
pyproject.toml (2)
215-216: Icon files are present and accessible.Both
doc/images/favicon.ico(16K) anddoc/images/dfetch_logo_only.png(389K) exist at the specified paths. The icon configuration is valid.
204-206: The resource inclusion settings in lines 204-206 are correct. Thedfetch/resourcesdirectory exists and contains the expected files (schema.yaml and template.yaml). Theinfer-licensedependency is declared in lines 53-54 with appropriate Python version constraints, and the explicit inclusion ofinfer_license.licensesis necessary for ensuring the module is available in the Nuitka standalone build.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pyproject.toml (1)
218-222: Add "windows-" prefix to all product metadata option names.The option names at lines 218-222 are missing the required "windows-" prefix. Nuitka expects these options in the
[tool.nuitka]section to be named:
windows-company-name(notcompany-name)windows-product-version(notproduct-version)windows-file-version(notfile-version)windows-product-name(notproduct-name)windows-file-description(notfile-description)Without the "windows-" prefix, these settings will be ignored by Nuitka.
🔎 Proposed fix
-company-name = "dfetch-org" -product-version = "{VERSION}" -file-version = "{VERSION}" -product-name = "dfetch" -file-description = "A vendoring tool for fetching and managing external dependencies." +windows-company-name = "dfetch-org" +windows-product-version = "{VERSION}" +windows-file-version = "{VERSION}" +windows-product-name = "dfetch" +windows-file-description = "A vendoring tool for fetching and managing external dependencies."
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
doc/images/dfetch_logo_only.pngis excluded by!**/*.pngdoc/images/dfetch_logo_only.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T22:46:27.554Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 861
File: dfetch/resources/__init__.py:12-17
Timestamp: 2025-12-25T22:46:27.554Z
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
⏰ 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). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: DevContainer Build & Test
- GitHub Check: build (windows-latest)
- GitHub Check: test-cygwin
🔇 Additional comments (2)
pyproject.toml (2)
202-202: Good fix! LTO option now uses correct string syntax.The
ltooption has been correctly updated to use the string value"auto"instead of a boolean. This enables automatic link-time optimization detection as expected by Nuitka's configuration parser.
215-216: Both icon files exist at the specified paths:doc/images/favicon.ico(16K) anddoc/images/dfetch_logo_only.png(389K).
pyproject.toml
Outdated
| include-data-dir="dfetch/resources=resources" | ||
| include-package-data="infer_license" | ||
| include-module="infer_license.licenses" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if dfetch/resources directory exists and list contents
echo "=== Checking dfetch/resources directory ==="
if [ -d "dfetch/resources" ]; then
echo "✓ dfetch/resources exists"
echo "Contents:"
find dfetch/resources -type f | head -20
else
echo "✗ dfetch/resources not found"
fi
# Check pyproject.toml dependencies
echo -e "\n=== Checking infer_license in dependencies ==="
grep -A 5 "infer_license" pyproject.toml | head -10
# Check if infer_license package structure exists
echo -e "\n=== Checking infer_license package ==="
fd -type f "infer_license" . --max-depth 3Repository: dfetch-org/dfetch
Length of output: 727
🏁 Script executed:
#!/bin/bash
# Check if infer_license is listed as a dependency in pyproject.toml
echo "=== Checking infer_license in pyproject.toml dependencies ==="
grep -B 5 -A 5 "infer.license\|infer-license" pyproject.toml | head -30
# Search for infer_license imports/usage in the codebase
echo -e "\n=== Searching for infer_license usage in codebase ==="
rg "infer.license\|infer-license\|from infer_license\|import infer_license" --type py
# Check the full dependencies section
echo -e "\n=== Full optional dependencies section ==="
sed -n '/\[project.optional-dependencies\]/,/^\[/p' pyproject.toml | head -50Repository: dfetch-org/dfetch
Length of output: 2312
🏁 Script executed:
#!/bin/bash
# Check if infer_license is used by any of the main dependencies
echo "=== Checking main dependencies that might use infer_license ==="
grep -E "^dependencies|^requires" pyproject.toml -A 30 | grep -E "name =|version =|infer"
# Check if there are any __init__.py or setup files that import infer_license
echo -e "\n=== Checking for dynamic imports or lazy loading ==="
rg "infer" --type py -i | head -20
# Check dfetch/__init__.py for any hidden imports
echo -e "\n=== Checking dfetch/__init__.py ==="
cat dfetch/__init__.py 2>/dev/null || echo "File not found"
# Check if cyclonedx-python-lib uses infer-license (since it's for SBOM generation)
echo -e "\n=== Checking what cyclonedx-python-lib is used for ==="
rg "cyclonedx" --type py -B 2 -A 2Repository: dfetch-org/dfetch
Length of output: 6009
🏁 Script executed:
#!/bin/bash
# Check if we can see what modules are in infer_license package
echo "=== Checking infer_license package structure ==="
python3 <<'EOF'
try:
import infer_license
import inspect
import pkgutil
# Get the package path
pkg_path = inspect.getfile(infer_license)
print(f"infer_license location: {pkg_path}")
# List all modules in infer_license
print("\nModules/subpackages in infer_license:")
if hasattr(infer_license, '__path__'):
for importer, modname, ispkg in pkgutil.iter_modules(infer_license.__path__):
print(f" - {modname} (package: {ispkg})")
# Try to import the actual modules used in dfetch
print("\nTesting imports used in dfetch code:")
try:
from infer_license.types import License
print(" ✓ infer_license.types.License - EXISTS")
except ImportError as e:
print(f" ✗ infer_license.types.License - {e}")
try:
import infer_license.api
print(" ✓ infer_license.api - EXISTS")
except ImportError as e:
print(f" ✗ infer_license.api - {e}")
try:
import infer_license.licenses
print(" ✓ infer_license.licenses - EXISTS")
except ImportError as e:
print(f" ✗ infer_license.licenses - DOES NOT EXIST: {e}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: dfetch-org/dfetch
Length of output: 148
Verify include-module directive for infer_license.
The dfetch/resources directory is confirmed to exist with necessary YAML files, and the mapping to top-level resources is correct.
However, infer_license is actively used in dfetch/util/license.py, but only imports:
infer_license(top-level)infer_license.types.Licenseinfer_license.api.probabilities()
The include-module="infer_license.licenses" directive on line 206 references a module that is not imported or used anywhere in the codebase. Verify whether:
infer_license.licensesis required as a transitive dependency for the modules above, or- This should reference
infer_license.typesorinfer_license.apiinstead, or - The directive can be removed entirely.
🤖 Prompt for AI Agents
In pyproject.toml around lines 204-206, the
include-module="infer_license.licenses" entry references a module not imported
anywhere; inspect the codebase for actual infer_license usage (you already noted
imports of infer_license, infer_license.types, and infer_license.api) and either
replace the include-module with the correct module(s) (e.g., infer_license.types
and/or infer_license.api) if they are needed at package build time, or remove
the include-module line entirely if none of those submodules are required; after
changing, run a local build/check to confirm no missing-module packaging errors.
There was a problem hiding this comment.
This is because this folder contains all the license files that infer license uses. The others are auto discovered by nuitka, but the resources aren't
There was a problem hiding this comment.
@spoorcc, thank you for the clarification! That makes perfect sense - infer_license.licenses contains the license text files (resources) that the package needs at runtime, and Nuitka doesn't auto-discover resource directories the same way it discovers Python modules. The explicit include-module directive ensures these license files are bundled in the standalone binary, similar to how include-data-dir is needed for dfetch/resources.
✏️ Learnings added
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 898
File: pyproject.toml:204-206
Timestamp: 2026-01-01T08:56:36.734Z
Learning: In dfetch's Nuitka configuration, `include-module="infer_license.licenses"` is required because this submodule contains license text files (resources) that infer_license needs at runtime. While Nuitka auto-discovers other infer_license modules like `infer_license.types` and `infer_license.api`, it does not auto-discover resource directories, so they must be explicitly included.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 861
File: dfetch/resources/__init__.py:12-17
Timestamp: 2025-12-25T22:46:27.554Z
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
script/package.py (1)
200-212: Good ARP metadata enhancements with minor suggestion.The ARP metadata additions improve the Windows installer's Add/Remove Programs presentation. The comment on line 210 clearly documents the intent.
Note: If any of the URL values (DOCS_URL, ISSUES_URL, CHANGELOG_URL) are empty strings, they'll still be set as WiX properties with empty values. While this won't break the build, you might consider conditionally adding these properties only when the URLs are non-empty.
🔎 Optional enhancement to conditionally set URL properties
# Add / Remove programs info (ARP) - ET.SubElement(package, "Icon", Id="AppIcon", SourceFile=str(WINDOWS_ICO_PATH)) - ET.SubElement(package, "Property", Id="ARPPRODUCTICON", Value="AppIcon") + if WINDOWS_ICO_PATH and WINDOWS_ICO_PATH.exists(): + ET.SubElement(package, "Icon", Id="AppIcon", SourceFile=str(WINDOWS_ICO_PATH)) + ET.SubElement(package, "Property", Id="ARPPRODUCTICON", Value="AppIcon") ET.SubElement(package, "Property", Id="ARPCOMMENTS", Value=DESCRIPTION) - ET.SubElement(package, "Property", Id="ARPURLINFOABOUT", Value=URL) - ET.SubElement(package, "Property", Id="ARPREADME", Value=DOCS_URL) - ET.SubElement(package, "Property", Id="ARPHELPLINK", Value=ISSUES_URL) - ET.SubElement(package, "Property", Id="ARPURLUPDATEINFO", Value=CHANGELOG_URL) + if URL: + ET.SubElement(package, "Property", Id="ARPURLINFOABOUT", Value=URL) + if DOCS_URL: + ET.SubElement(package, "Property", Id="ARPREADME", Value=DOCS_URL) + if ISSUES_URL: + ET.SubElement(package, "Property", Id="ARPHELPLINK", Value=ISSUES_URL) + if CHANGELOG_URL: + ET.SubElement(package, "Property", Id="ARPURLUPDATEINFO", Value=CHANGELOG_URL) # Don't show modify & repair buttons, only remove ET.SubElement(package, "Property", Id="ARPNOMODIFY", Value="1") ET.SubElement(package, "Property", Id="ARPNOREPAIR", Value="1")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyproject.tomlscript/package.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
⏰ 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). (19)
- GitHub Check: Publish Python distribution 📦 to TestPyPI
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (macos-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: test-cygwin
- GitHub Check: DevContainer Build & Test
- GitHub Check: build (windows-latest)
🔇 Additional comments (1)
script/package.py (1)
22-24: LGTM!The URL extraction from pyproject.toml follows the same pattern as the existing code and is implemented correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
script/package.py (1)
34-38: Icon file existence validation is still missing.While the code now handles empty strings by setting
WINDOWS_ICO_PATHtoNone, it doesn't validate that the file exists when a path is provided. If the icon file is missing, the WiX build will fail with an unclear error at Line 208.🔎 Enhance the validation to check file existence
tools = pyproject.get("tool", {}) nuitka_info = tools.get("nuitka", {}) WINDOWS_ICO = nuitka_info.get("windows-icon-from-ico", "") -WINDOWS_ICO_PATH = Path(WINDOWS_ICO).resolve() if WINDOWS_ICO else None +if WINDOWS_ICO: + WINDOWS_ICO_PATH = Path(WINDOWS_ICO).resolve() + if not WINDOWS_ICO_PATH.exists(): + print(f"Warning: Windows icon file not found: {WINDOWS_ICO_PATH}") + WINDOWS_ICO_PATH = None +else: + WINDOWS_ICO_PATH = NoneBased on the past review comment, this validation was requested to prevent unclear WiX build failures.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/package.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). (20)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.9)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (ubuntu-latest, 3.14)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (macos-latest, 3.9)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (ubuntu-latest, 3.11)
- GitHub Check: test
- GitHub Check: test-cygwin
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (2)
script/package.py (2)
22-24: LGTM! URL extraction is safe.The extraction of additional URLs from
pyproject.tomlis implemented correctly with safe defaults.
212-213: LGTM! Standard ARP configuration.Disabling modify and repair buttons is a standard practice for simple installers that only support installation and removal.
|
Installs correctly on windows: 
Shown with icon in Add / remove programs: 
Info is added: 
Seen in PATH: Works after install (basic test) 
|
add ico to windows generated icon
Summary by CodeRabbit
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.