Skip to content

test branch for executables workflow#146

Open
sjswerdloff wants to merge 22 commits intomainfrom
sjswerdloff-patch-1
Open

test branch for executables workflow#146
sjswerdloff wants to merge 22 commits intomainfrom
sjswerdloff-patch-1

Conversation

@sjswerdloff
Copy link
Copy Markdown
Owner

@sjswerdloff sjswerdloff commented May 21, 2025

Summary by Sourcery

Enable end-to-end Windows executable builds with improved caching, security scanning, automated release note generation, and robust packaging; enhance configuration file lookup for bundled apps; and update tests and dependencies for Python 3.11+ compatibility

New Features:

  • Introduce an automated Windows executable build workflow with manual and main-branch triggers
  • Cache Poetry installation, Python dependencies, and PyInstaller artifacts with weekly rotation
  • Perform dependency security scans and generate release notes from recent commits
  • Create draft GitHub releases with packaged artifacts and 30-day retention

Bug Fixes:

  • Add fallback logic for locating config files when running as a bundled executable

Enhancements:

  • Rename workflow file and improve PowerShell packaging commands with error handling
  • Update tests to conditionally import tomllib on Python 3.11+ and import sys where needed
  • Add PyInstaller to project dependencies and bump ruff version in pre-commit config

Build:

  • Add a new build_executables.yml workflow for executable packaging

CI:

  • Expand workflow triggers to include pushes on main and relevant files

Tests:

  • Include sys import in tests and adjust toml parsing compatibility

sjswerdloff and others added 12 commits May 21, 2025 16:34
make tdwii_config look for configuration files relative to executable (sys.argv[0]) if __file__ isn't found
	which is the situation when the app is built in to an exe
add build-executables CI workflow (manual trigger)
correct to FileNotFoundError

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
removed platform-name and path-sep because they aren't being used.

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
corrected name of upload artefact

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai bot commented May 21, 2025

Reviewer's Guide

This PR integrates a robust Windows executable build pipeline via GitHub Actions with caching, security scans, and automated releases, extends application code to locate config files when bundled, adds PyInstaller as a dependency and updates lint tooling, and makes tests compatible with Python 3.11’s tomllib.

Sequence Diagram for Configuration File Fallback Logic

sequenceDiagram
    participant AppLogic as "Application Logic"
    participant FileSystem as "File System"
    participant ExecutionContext as "Execution Context (sys.executable)"

    AppLogic->>FileSystem: Attempt to resolve config file path (e.g., relative to __file__)
    alt Primary path resolution successful
        FileSystem-->>AppLogic: Config file found at primary path
        AppLogic->>AppLogic: Load config from primary path
    else Primary path resolution failed (e.g., __file__ context unavailable in bundle)
        FileSystem-->>AppLogic: Config file not found at primary path
        AppLogic->>ExecutionContext: Get executable directory
        ExecutionContext-->>AppLogic: Executable directory path
        AppLogic->>FileSystem: Attempt to resolve config file path (relative to executable directory)
        alt Fallback path resolution successful
            FileSystem-->>AppLogic: Config file found at fallback path
            AppLogic->>AppLogic: Load config from fallback path
        else Fallback path resolution failed
            FileSystem-->>AppLogic: Config file not found at fallback path
            AppLogic->>AppLogic: Raise FileNotFoundError
        end
    end
Loading

File-Level Changes

Change Details Files
Overhaul CI workflow to build, package, and release Windows executables
  • Renamed and merged build workflows; enabled on main branch and specific paths
  • Introduced multi-layer caching for Poetry, dependencies, and PyInstaller with weekly rotation
  • Added security scan of dependencies and automated release notes generation
  • Refined packaging steps and configured draft GitHub Release with explicit artifact lists
.github/workflows/alt_build_executables.yml
.github/workflows/build-executables.yml
Implement fallback logic for config file resolution in bundled executables
  • Detect missing file path and switch to sys.executable install directory
  • Attempt to locate config files under executable path before raising FileNotFoundError
  • Provide clear error message when neither internal nor external paths exist
tdwii_plus_examples/cli/upsscp/upsscp.py
tdwii_plus_examples/cli/upsscp/handlers.py
tdwii_plus_examples/tdwii_config.py
Update project dependencies and lint configuration
  • Added PyInstaller to all relevant sections in pyproject.toml
  • Bumped Ruff version in pre-commit-config.yaml to v0.11.10
pyproject.toml
.pre-commit-config.yaml
Enhance test suite for Python 3.11+ compatibility
  • Introduced conditional imports of tomllib vs tomli based on sys.version_info
  • Standardized noqa and formatting in multiple test modules
tdwii_plus_examples/tests/test_ppvs_toml.py
tdwii_plus_examples/tests/test_tdd_toml.py
tdwii_plus_examples/tests/test_rtbdi_toml.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sjswerdloff
Copy link
Copy Markdown
Owner Author

I worked out the details in this PR (after jamming in just the workflow in to main...).
So I cancelled the other PR and want to use this one.

@sjswerdloff sjswerdloff marked this pull request as ready for review May 21, 2025 10:21
@sjswerdloff sjswerdloff requested a review from dwikler May 21, 2025 10:21
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @sjswerdloff - I've reviewed your changes - here's some feedback:

  • You now have two nearly identical GitHub workflow files (alt_build_executables.yml and build-executables.yml); consider consolidating them to avoid duplication and simplify maintenance.
  • The tomli/tomllib import fallback is copy-pasted across many tests—extract it into a shared helper or pytest fixture to DRY up your test suite.
  • Your complex cache key (including branch-id and weekly rotation) may reduce cache hits; consider simplifying the key to improve cache reuse across runs.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain to me how starting around line #389 you can tell if it's a relative path in the configuration?
This needs to change to something like:

if getattr(sys, 'frozen', False):
        current_dir = os.path.dirname(sys.executable)
    elif __file__:
        current_dir = os.path.dirname(__file__)

    # current_dir = os.path.abspath(os.path.dirname(__file__))

for a pyinstaller .exe
One of the Issues I created was consolidation of this type of logic (it's a bit of a hack elsewhere, but I think this code fragment is the right way to get this information).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime, I just configured an absolute path in the upsscp ini file and that worked.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The os.path.join takes care of this. If the second arg is an absolute path, it does ignore the first arg.
So with:

    # Set the instance storage and database directories to current directory
    # if setting is not an absolute path
    current_dir = os.path.abspath(os.path.dirname(__file__))
    instance_dir = os.path.join(current_dir, app_config["instance_location"])
    instance_dir_path = os.path.abspath(instance_dir)

instance_dir will be either

  • ./tdwii_plus_examples/cli/upsscp/<instance_location> if <instance_location> is a relative path or
  • <instance_location> if <instance_location> is an absolute path

The 3rd line is actually useless as in both cases instance_dir will be an absolute path.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree there is a need for a change when packaging the scripts into executables as the file is not valid without a script file.
The proposed change should work to get current_dir and use it in the second line of the code block above to get the absolute path using the config parameter whether it is absolute or relative to the executable location.

@sjswerdloff sjswerdloff self-assigned this May 21, 2025
@sjswerdloff
Copy link
Copy Markdown
Owner Author

Hey @sjswerdloff - I've reviewed your changes - here's some feedback:

  • You now have two nearly identical GitHub workflow files (alt_build_executables.yml and build-executables.yml); consider consolidating them to avoid duplication and simplify maintenance.
  • The tomli/tomllib import fallback is copy-pasted across many tests—extract it into a shared helper or pytest fixture to DRY up your test suite.
  • Your complex cache key (including branch-id and weekly rotation) may reduce cache hits; consider simplifying the key to improve cache reuse across runs.

Here's what I looked at during the review
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

I cancelled the other workflow.
I created an issue for the tomllib duplications
I created an issue for the cacheing in the build exe workflow.

fix bug in handling ScanSpotMetersetWeights (list vs. float for single spot) when generating treatment record
added tdd to build of executables
removed files from release that won't be there, only using the zip (might be worth revisiting the packaging approach)
@dwikler
Copy link
Copy Markdown
Collaborator

dwikler commented Jun 2, 2025

I did clone the branch to try the executables on my Windows 11 minipc but I did not find the dist directory, I don't see it in the branch code on GitHub either.
I did try to generate the executables myself by adding pyinstaller to poetry and running all commands from the alt_build_executables.yml GitHub action "Build with PyInstaller (Windows)" step.
I have the following errors running the executables:

λ tdwii_config_dump
Traceback (most recent call last):
  File "config_dump.py", line 42, in <module>
  File "config_dump.py", line 27, in main
  File "tdwii_plus_examples\tdwii_config.py", line 18, in load_ae_config
FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\david\\AppData\\Local\\Temp\\_MEI48442\\tdwii_plus_examples\\config\\ApplicationEntities.json'
[PYI-15460:ERROR] Failed to execute script 'config_dump' due to unhandled exception!

image

Copy link
Copy Markdown
Collaborator

@dwikler dwikler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left comments in the pull request after trying to run the executables and experiencing some errors. Adding a markdown file to explain how to download or build the executables may help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants