Replace aw-notify with Rust implementation aw-notify-rs#1211
Replace aw-notify with Rust implementation aw-notify-rs#1211ErikBjare merged 2 commits intoActivityWatch:masterfrom
Conversation
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 22c0ee6 in 5 seconds. Click for details.
- Reviewed
41lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_kx3k3drzyWAyyRMT
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Greptile SummaryThis PR replaces the Python-based Key Changes:
No Issues Found:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 22c0ee6 |
|
The macOS CI failures here ( Root cause: Apple Developer Agreement has expired — This PR itself looks great — both Ellipsis and Greptile give it a thumbs up (5/5 confidence). Once Erik resolves the Apple Developer account issue, the macOS jobs should pass. In the meantime, the PR is ready from a code quality perspective. |
|
Correction to my previous comment: I was wrong about the macOS failures being unrelated. The actual root cause:
Since this PR replaces Python Fix needed in # Check if aw-notify is a Python package
_notify_candidates = [
aw_notify_location / "aw_notify/__main__.py",
aw_notify_location / "src/aw_notify/__main__.py",
]
skip_aw_notify = not any(p.exists() for p in _notify_candidates)
if skip_aw_notify:
print("Skipping aw-notify Python packaging (Rust-based implementation detected)")
aw_notify_a = None if skip_aw_notify else build_analysis(
"aw_notify", aw_notify_location, hiddenimports=["desktop_notifier.resources"]
)Then wrap all subsequent uses ( The Rust binary itself ( |
|
@0xbrayo Could you move aw-notify-rs into the ActivityWatch org? :) |
|
transferred to AW |
|
@TimeToBuildBob reupdate the links, now aw-notify-rs is located at activitywatch/aw-notify-rs |
|
@0xbrayo Now that aw-notify-rs is at
Also make sure the submodule commit points to the new repo's commit (you may need to |
22c0ee6 to
1ee371c
Compare
|
@0xbrayo Fixed the Apple Notarization stuff (just needed to accept new ToS), re-triggered the CI. |
|
@TimeToBuildBob Is the remaining failing CI just a consequence of running from Brayo's fork? Safe to merge? Or do we still need to do something about #1211 (comment)? (also, I think Brayo gave him access to his fork, unless that was just aw-tauri, so you should maybe be able to push directly to it and fix this instead of telling us to) |
- Check if aw-notify is a Python package before building - Skip Python packaging when Rust implementation (aw-notify-rs) is detected - Conditionally include aw-notify in MERGE and BUNDLE operations - Prevents build failures when aw-notify submodule is replaced with aw-notify-rs Implements suggestions from PR ActivityWatch#1211 comment: ActivityWatch#1211 (comment)
|
@ErikBjare To answer your questions:
Options to unblock:
I'd recommend option 1 or 2. @0xbrayo could you grant @TimeToBuildBob push access to your activitywatch fork, or apply the |
|
@TimeToBuildBob I have sent you an invite to be a collaborator on my fork. |
|
@TimeToBuildBob wait for the CI builds to finish, seems like the last commit resolved the errors, except for a transient integration error on ubuntu |
|
The `aw.spec` fix has been pushed (commit 0367d2d). Here's the current CI summary: macOS jobs: Both `macos-14` and `macos-latest` are now passing — the `aw.spec` guard is working correctly. Remaining failure: `ubuntu-24.04 / Run integration tests` fails with: Two `windows-latest` jobs are still pending (runners queued). Short summary: the actual fix for this PR (aw.spec + submodule + README) is complete and working. The one failing job is a known flaky test unrelated to aw-notify-rs. Safe to merge once the Windows jobs complete. |
|
Windows jobs are both green now. All CI passes except the one pre-existing flaky Ready to merge. |
|
Merging. @TimeToBuildBob investigate the flaky test and address in seperate PR. |
The test was intermittently failing with `assert 9 == 8` because: 1. `midnight` retained residual microseconds from `datetime.now()`, making the exact midnight boundary non-deterministic across runs 2. Event 9 (starting at 23:59 with 1-min duration) has endtime exactly at midnight, and the server's `endtime >= start_query` condition sometimes includes it in the "after midnight" query results Fix: zero out seconds/microseconds on the start timestamp for deterministic boundaries, and add a 1ms offset to the query start to avoid the ambiguous boundary where events ending exactly at midnight could be included. Closes ActivityWatch/activitywatch#1211 (follow-up)
|
Investigated the flaky Root cause: The test creates 20 events spanning midnight (23:50→00:09), then queries events after midnight. Event 9 (at 23:59, duration 1min) has Fix: Zero out seconds/microseconds for deterministic timestamps + add 1ms query offset to avoid the boundary ambiguity. |
…157) * fix(test): resolve flaky test_midnight_heartbeats boundary condition The test was intermittently failing with `assert 9 == 8` because: 1. `midnight` retained residual microseconds from `datetime.now()`, making the exact midnight boundary non-deterministic across runs 2. Event 9 (starting at 23:59 with 1-min duration) has endtime exactly at midnight, and the server's `endtime >= start_query` condition sometimes includes it in the "after midnight" query results Fix: zero out seconds/microseconds on the start timestamp for deterministic boundaries, and add a 1ms offset to the query start to avoid the ambiguous boundary where events ending exactly at midnight could be included. Closes ActivityWatch/activitywatch#1211 (follow-up) * style: fix Black formatting in test_midnight_heartbeats
Important
Replace
aw-notifywithaw-notify-rsin submodules and updateREADME.mdaccordingly.aw-notifysubmodule withaw-notify-rsin.gitmodules.README.mdto linkaw-notifyto0xbrayo/aw-notify-rsin the architecture diagram.This description was created by
for 22c0ee6. You can customize this summary. It will automatically update as commits are pushed.