-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Copy executables to Python virtual environment #500
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to d885b30 in 1 minute and 57 seconds
More details
- Looked at
87
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. Makefile:5
- Draft comment:
Consider using 'python3' instead of 'python' to ensure consistency on systems where Python 3 is required. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While using python3 explicitly can be good practice, this seems like an implementation detail that would be caught immediately if it was an issue. If 'python' doesn't point to Python 3 on the system, the build would fail immediately and obviously. Most modern systems alias python to python3 by default. The comment doesn't indicate a clear problem that needs fixing.
The comment could be valid if the project has specific Python 3 requirements or if there are systems where this could cause compatibility issues.
If Python 3 was required, the build would fail immediately and obviously. This is more of a system configuration issue than a code issue.
Delete the comment as it's not addressing a clear problem that needs fixing. The build system would catch any Python version incompatibility immediately.
2. Makefile:111
- Draft comment:
Consider using 'python3' to call the cleanup script for consistency with the build target. - Reason this comment was not posted:
Marked as duplicate.
3. scripts/copy_rust_binaries.py:11
- Draft comment:
In is_executable() consider checking the OS first to simplify the logic for Windows-specific extensions. - Reason this comment was not posted:
Confidence changes required:50%
None
4. Makefile:5
- Draft comment:
Use a version-specific interpreter (e.g. 'python3' or a $(PYTHON) variable) instead of 'python' to avoid ambiguity on systems where 'python' might point to Python 2. - Reason this comment was not posted:
Marked as duplicate.
5. Makefile:112
- Draft comment:
Use 'python3' (or a defined $(PYTHON) variable) instead of 'python' here as well to ensure the correct interpreter is used. - Reason this comment was not posted:
Marked as duplicate.
6. scripts/copy_rust_binaries.py:57
- Draft comment:
Specify nargs='?' for the positional 'target_dir' argument so that the default value is used when omitted. - Reason this comment was not posted:
Marked as duplicate.
7. scripts/copy_rust_binaries.py:38
- Draft comment:
On Windows, the is_executable() fallback does not check for file existence. Verify the file exists before calling os.remove() to avoid errors. - Reason this comment was not posted:
Comment did not seem useful.
8. scripts/copy_rust_binaries.py:20
- Draft comment:
Consider printing the error message to sys.stderr instead of stdout for better error reporting. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_JcNlOaEWeOdtGKmm
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
scripts/copy_rust_binaries.py
Outdated
parser = argparse.ArgumentParser() | ||
|
||
parser.add_argument("--clean", action='store_true', default=False) | ||
parser.add_argument("target_dir", type=str, default=os.path.join("target", "release")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Positional argument 'target_dir' provides a default value but lacks nargs='?'; this may prevent the default from being used if omitted.
scripts/copy_rust_binaries.py
Outdated
def build(target_dir, python_bin_dir): | ||
|
||
if not os.path.exists(target_dir): | ||
print(f"Error: {target_dir} does not exist. Did you run `cargo build --release`?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider writing the error message to stderr instead of stdout when the target directory doesn't exist.
d885b30
to
9f526fa
Compare
The alternative would be to update the docs with this: Build and install everything into the virtualenv:
.. code-block:: sh
make build
cp aw-server-rust/target/release/aw-server venv/bin/aw-server-rust # optional
.. note::
If you want to use `aw-server-rust` with `aw-qt`, you'll want to copy its binary into your `venv` |
This is essentially what |
Rationale:
If you don't like this way of moving the binaries, would you prefer using Poetry to install them? |
Or use Poetry to install a small Python script that invokes the rust executable? |
8cb42d8
to
4a3b0b6
Compare
4a3b0b6
to
691a4ab
Compare
This sets the Makefile to copy
aw-server-rust
(and other executables appending -rust suffix to basename) to the virtualenv (or Python scripts folder) to make the build process more intuitive. Allowsaw-server-rust
to be run from terminal when the Python environment (i.e. virtualenv) used to install ActivityWatch is active, and simplifies usingaw-qt
withaw-server-rust
without added documentation and extra steps.As it is the user has to go into
aw-server-rust
, docp target/release/aw-server ../venv/bin/aw-server-rust
manually per the README in order to getaw-server-rust
available underaw-qt
. Or the rust server is executed in a different path from other modules.Related to ActivityWatch/aw-qt#107 as aw-server-rust is now the preferred server, which is not documented yet.
Important
Automates copying of Rust executables to Python virtual environment in the build process, simplifying integration with
aw-qt
.Makefile
updated to runscripts/copy_rust_binaries.py
duringbuild
andclean
targets.-rust
to the basename.aw-server-rust
is available in the Python environment when active.scripts/copy_rust_binaries.py
added.build()
function copies executables fromtarget_dir
to Python scripts directory.clean()
function removes copied executables from Python scripts directory.sysconfig
to determine Python scripts directory.aw-server-rust
the preferred server inaw-qt
.This description was created by
for d885b30. It will automatically update as commits are pushed.