Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Game4Move78
Copy link

@Game4Move78 Game4Move78 commented Feb 3, 2025

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. Allows aw-server-rust to be run from terminal when the Python environment (i.e. virtualenv) used to install ActivityWatch is active, and simplifies using aw-qt with aw-server-rust without added documentation and extra steps.

As it is the user has to go into aw-server-rust, do cp target/release/aw-server ../venv/bin/aw-server-rust manually per the README in order to get aw-server-rust available under aw-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.

  • Behavior:
    • Makefile updated to run scripts/copy_rust_binaries.py during build and clean targets.
    • Copies Rust executables to Python virtual environment, appending -rust to the basename.
    • Ensures aw-server-rust is available in the Python environment when active.
  • Scripts:
    • New script scripts/copy_rust_binaries.py added.
    • build() function copies executables from target_dir to Python scripts directory.
    • clean() function removes copied executables from Python scripts directory.
    • Uses sysconfig to determine Python scripts directory.
  • Misc:
    • Related to making aw-server-rust the preferred server in aw-qt.

This description was created by Ellipsis for d885b30. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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.

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"))
Copy link

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.

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`?")
Copy link

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.

@Game4Move78
Copy link
Author

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`

@ErikBjare
Copy link
Member

ErikBjare commented Feb 3, 2025

This is essentially what make package already does? I don't like the idea of moving binaries into the venv like this.

@Game4Move78
Copy link
Author

make package moves them to dist. This script makes them available on PATH by installing them to the same common venv used by every other module for storing binaries.

Rationale:

  • The aw-server-rust README suggests copying the binary into the venv
  • Installing into the venv ensures the binaries are accessible via PATH while keeping everything contained within the virtual environment.
  • Maintains consistency with other modules that install binaries into the venv via poetry install
  • Without this change, aw-server is readily available on PATH, but the preferred aw-server-rust is not

If you don't like this way of moving the binaries, would you prefer using Poetry to install them?

@Game4Move78
Copy link
Author

Or use Poetry to install a small Python script that invokes the rust executable?

@Game4Move78 Game4Move78 force-pushed the copy-target-venv branch 4 times, most recently from 8cb42d8 to 4a3b0b6 Compare February 3, 2025 22:25
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