feat:add BUILD_SYNC_ONLY variable to makefile#540
feat:add BUILD_SYNC_ONLY variable to makefile#5400xbrayo wants to merge 1 commit intoActivityWatch:masterfrom
Conversation
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 5b9c3e9 in 58 seconds. Click for details.
- Reviewed
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft 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.
1. Makefile:1
-
Draft comment:
PR title mentions BUILD_SYNC_ONLY variable but no related logic is present in the diff. Please update the title/description or include the variable implementation. -
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. Makefile:34
-
Draft comment:
The compound condition 'ifneq ($(or $ (filter true,$(SKIP_WEBUI)),$(filter true,$(SKIP_RUST_SERVER))),)' is valid but a brief inline comment explaining its intent would improve maintainability. -
Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. Makefile:95
-
Draft comment:
In the package target, the conditional check for SKIP_RUST_SERVER is correct, but adding an inline comment to clarify that the server binary is omitted from the package when SKIP_RUST_SERVER is true would be beneficial. -
Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
4. Makefile:24
-
Draft comment:
The conditional check styles differ across targets (aw-server uses 'ifeq' while aw-webui uses a compound 'ifneq'). Consider unifying these for consistency and readability. -
Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_SdY8nlmn9tz3M9CD
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #540 +/- ##
==========================================
- Coverage 70.81% 70.15% -0.66%
==========================================
Files 51 51
Lines 2916 2939 +23
==========================================
- Hits 2065 2062 -3
- Misses 851 877 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This for packaging with |
|
This is a Just to note when squash merging |
|
Maybe just create a different Makefile target to build aw-sync? I don't really think it makes sense to skip building the server for "make build" in the server. Maybe add a Makefile to the aw-sync subdir with such a target. |
|
I think I'll do |
Important
Add
SKIP_RUST_SERVERvariable toMakefileto conditionally skip building and packagingaw-server.SKIP_RUST_SERVERvariable inMakefileto conditionally skip buildingaw-server.aw-webuibuild condition to skip ifSKIP_RUST_SERVERis true.packagetarget to conditionally copyaw-serverbinary based onSKIP_RUST_SERVER.This description was created by
for 5b9c3e9. You can customize this summary. It will automatically update as commits are pushed.