-
-
Notifications
You must be signed in to change notification settings - Fork 585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace aw-server-python with aw-server-rust #331
Conversation
34063b4
to
c35bae5
Compare
Could you please avoid removing aw-server-python and keep both in the repo so that this PR can be merged even though we're not ready to completely move over to aw-server-rust? Building the bundle with both for a release or two would also let users test it before we switch over. |
Also, apparently the Python 2.7 on macOS has to do with the aw-core (and maybe other) Makefile using |
How do you expect that to work? aw-qt will start both automatically if it's packaged properly.
My idea was that it might be a better idea to just release "actual" beta versions. |
We'd simply make aw-qt not start aw-server-rust by default, or perhaps even better make it a configurable which one to use (until that is done, you can specify it by giving aw-qt the I want this for testing purposes regardless (especially because of #327). When we in the future want to exclude aw-server-python from builds, I suggest we do that with a CI build flag instead.
I'd rather not honestly. If so I'd expect us to have a separate branch for it, and maintaining a separate release channel/branch is too much work with our submodule-based repo layout (if we had a monorepo that'd be a different story). |
After re-adding aw-server-python and taking a look at aw-qt I realize that it would not for two reasons:
|
Travis doesn't terminate on the first non-zero exitcode step in |
CI should work now, apart from the failing aw-server-rust integration test. |
Fair enough
If aw-server-rust was a WIP I'd agree, but all features are working and have done so for almost a year so there's only bug hunting and incompatibilities with watchers left to investigate.
While the name of the git is "aw-server-rust" the name of the binary is now just "aw-server" since I split it up into modules, if we do it like this we need to manually rename the binary in the Makefile.
Oh, just realized that ActivityWatch/aw-qt#27 never got finished, didn't know.
Huh, that's... interesting behavior to say the least. I'd expect it to do "set -e" by default but apparently not. |
I did, in 0aec062.
The idea is to let it run tests/linting/typechecking/whatever such that if more than one fails you can fix them all in one go. |
@ErikBjare I'm starting a windows VM now to try and fix the blinking test. |
@johan-bjareholt How did that blinking test go? Want me to take a look at it? |
@ErikBjare I fixed and merged it this weekend, works fine! There's one thing failing in Travis in the aw-server-rust Repo and that's some Travis caching mechanism taking more that 10min on macOS (so that shouldn't impact this repo as it's specific to projects marked as rust which this isn't). Otherwise tests are reliable now as far as I know. |
70abf85
to
46aa6dd
Compare
Aw-qt errors for windows now and some strange network error for macOS |
The macOS network error is just a spurious error. I restarted the build. Seems like the perl regex that's used as a workaround for a poetry issue with packing the One other thing though, even though it is failing the build also seems to timing out. Is aw-server-rust correctly shut down in the integration tests? |
Yes, aw-server is compiled into the aw-client-rust tests so they run as the same process. When the main thread shuts down (the test thread) the server itself shuts down with it. Not the cleanest way (as there is no way to cleanly shut down a rocket server apparently), but it does what it should. Kind of odd as in appveyor it shuts down properly in 15-20min while travis times out after 50min, strange. |
It's something with the .travis.yml stdout timeout thing not shutting down properly probably
|
@ErikBjare Any progress on this? |
Maybe just replacing perl with sed would fix it? |
@johan-bjareholt Unfortunately sed has platform differences between macOS and Linux which makes it difficult to use in CI, which is why I switched to perl in the first place: https://stackoverflow.com/questions/4247068/sed-command-with-i-option-failing-on-mac-but-works-on-linux Things seem to work in the master branch, so maybe merging it into this PR will fix it?
I don't think so, works fine on master. |
46aa6dd
to
d2aadf0
Compare
I rebased instead so we avoid potentially breaking master |
…rver-rust submodule to correctly name executable as aw-server-rust
d2aadf0
to
2b1597e
Compare
The unstructured commits in the previous version changed the aw-qt version 3 times. |
2b1597e
to
2ca1e6e
Compare
Replaced perl with grep in ActivityWatch/aw-qt#53 Hopefully this will work |
Finally, it's all green! Merging! 🎆 |
To do: