Skip to content

Conversation

@BelKed
Copy link
Contributor

@BelKed BelKed commented Mar 12, 2025

The size of the built file has changed the number of digits, and manual parsing by splitting on spaces doesn't work anymore


Important

Replaces manual file size parsing with awk in Makefile to ensure accurate file size comparison in CI reproducibility tests.

  • Behavior:
    • Replaces manual file size parsing with awk in test-reproducibility-chrome and test-reproducibility-firefox in Makefile.
    • Ensures accurate file size comparison regardless of digit count.
  • Misc:
    • Removes sorting and unique counting logic from reproducibility tests.

This description was created by Ellipsis for bf53ce0. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to bf53ce0 in 45 seconds

More details
  • Looked at 37 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. Makefile:73
  • Draft comment:
    Good usage of awk to isolate the file size. Consider quoting file paths (e.g., "artifacts/chrome.zip") to safeguard against potential spaces in filenames.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
2. Makefile:84
  • Draft comment:
    Repeat of the file size comparison for Firefox. Suggest quoting file path arguments to avoid issues if paths contain spaces.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
3. Makefile:73
  • Draft comment:
    Nice improvement using wc and awk to compare file sizes rather than brittle manual parsing. Consider also that wc output is padded; while awk '{print $1}' removes whitespace, you might alternatively use something like 'stat -c%s' (or 'stat -f%z' on macOS) for a more direct file size value. Also, since similar logic is used for both chrome and firefox tests, extracting this into a macro or function could improve maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
4. Makefile:84
  • Draft comment:
    The firefox test now uses the same robust approach for file size comparison. As with the chrome test, consider deduplicating this logic and, if appropriate, exploring alternatives like 'stat' for clarity, keeping portability in mind.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None

Workflow ID: wflow_W6sL6qB3zuYclJHA


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ErikBjare
Copy link
Member

Ahh! I was really wondering what was happening there.

Thank you so much 🙏 ❤️

@ErikBjare ErikBjare merged commit febd76a into ActivityWatch:master Mar 12, 2025
5 checks passed
@BelKed BelKed deleted the fix-ci branch March 12, 2025 14:20
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