Skip to content

Conversation

@0xbrayo
Copy link
Contributor

@0xbrayo 0xbrayo commented Jul 25, 2025

No description provided.

@0xbrayo
Copy link
Contributor Author

0xbrayo commented Jul 25, 2025

This for building awatcher in the parent activitywatch repo. It will be bundled and distributed as part of activitywatch on linux.

@2e3s
Copy link
Owner

2e3s commented Jul 25, 2025

Hello! Could you please expand the context more, maybe there is a related discussion? Or a standard/expectations to comply? From my point of view, cargo build does the complete job, I don't understand this makefile.

@0xbrayo
Copy link
Contributor Author

0xbrayo commented Jul 25, 2025

It's not 💯 necessary. It is for the Makefile in activitywatch/activitywatch so that it can be built as "seemlessly" with the other modules. I can maintain a fork of your repo that has the extra Makefile should you not want to have it in your repo.

@2e3s
Copy link
Owner

2e3s commented Jul 25, 2025

I can maintain a fork of your repo that has the extra Makefile

I think it wouldn't be optimal since you'd need to keep it up to date with all bug fixes and whatnot.
Alright, I've read the makefile by your link and now I understand what you mean, thanks. Clippy is failing probably because of the Rust version update, I'll fix the failures later after a merge.

On a side note, in my opinion, ActivityWatch build system violates a single responsibility principle: it holds its submodules responsible for a correct build structure which it expects, coupled to sort of Makefile API contract. Let's imagine a case when the build system gets improved with a new functionality, e.g. replaced build command with build-slim and build-debug. And now you need to go to every repository and make a PR with a change to its Makefile. And that submodule might've had its own build-slim for its own purpose.
In order to achieve higher modular cohesion, that overaching multi-module repository which you refer should rather have makefile scripts for every submodule, and use them like $ make -f ./makefiles/awatcher.makefile --directory=./awatcher build. This awatcher.makefile may or may not use module's Makefile, or straightly cargo build, depending on the module's own build system. Optionally, the script can try to fall back to the module's Makefile if the parental {module}.makefile is not found, if module already has a properly structured Makefile. Git modules are bound by commit hash to the parent repository, so it's safely independent.
What do you think? Are there downsides? Implementation would be very trivial, just a couple more ifs in that overaching Makefile.

@0xbrayo
Copy link
Contributor Author

0xbrayo commented Jul 25, 2025

awatcher is the first module that is not part of ActivityWatch org that was included in the repo. I like your idea for sure just not sure if Erik would agree given that the possibility of ActivityWatch adding another module is slim. I will make sure to mention it. Thanks for maintaining awatcher btw.

@2e3s
Copy link
Owner

2e3s commented Jul 25, 2025

Yes, I understand that you aren't the author. I won't constrain you and merge this makefile if my suggestion won't be approved and implemented.

the first module that is not part of ActivityWatch org that was included in the repo

But my argumentation would apply to any module, it's a general design/pattern which boils down to coupling vs cohesion, dependency inversion et al.

@2e3s
Copy link
Owner

2e3s commented Jul 28, 2025

@0xbrayo please, as soon as you know if the build logic changes, tell me need I merge this PR or not 🙏

@0xbrayo
Copy link
Contributor Author

0xbrayo commented Jul 28, 2025

I will let you know :)

@0xbrayo 0xbrayo force-pushed the main branch 4 times, most recently from a4f6a53 to d6fdd6b Compare July 30, 2025 10:32
@0xbrayo
Copy link
Contributor Author

0xbrayo commented Aug 12, 2025

hi, I guess we will merging this, would require too much of a rework. We might reconsider it if the number of modules keeps growing. Thanks :)

@2e3s
Copy link
Owner

2e3s commented Aug 13, 2025

I think I would be able to make a very small PR with my idea, but anyway.

@2e3s 2e3s merged commit 2e3d97e into 2e3s:main Aug 13, 2025
3 of 4 checks passed
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