Bug 2237084 (espanso) - Review Request: espanso - Cross-platform Text Expander written in Rust
Summary: Review Request: espanso - Cross-platform Text Expander written in Rust
Keywords:
Status: ASSIGNED
Alias: espanso
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: rust-include_dir rust-markdown rust-log-panics
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-09-03 15:37 UTC by Robert-André Mauchin 🐧
Modified: 2023-12-17 19:23 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:
decathorpe: fedora-review?


Attachments (Terms of Use)

Description Robert-André Mauchin 🐧 2023-09-03 15:37:30 UTC
Spec URL: https://eclipseo.fedorapeople.org/for-review/rust-espanso.spec
SRPM URL: https://eclipseo.fedorapeople.org/for-review/rust-espanso-2.2.0-1.fc38.src.rpm

Description:
Cross-platform Text Expander written in Rust.  A text expander is a program that detects when you type a specific keyword and replaces it with something else. This is useful in many ways:   - Save a lot of typing, expanding common sentences.  - Create system-wide code snippets.  - Execute custom scripts  - Use emojis like a pro.

Fedora Account System Username: eclipseo

To build it against the dependencies, use the following COPR in your rawhide mock.cfg:

[copr:copr.fedorainfracloud.org:eclipseo:espanso]
name=Copr repo for espanso owned by eclipseo
baseurl=https://download.copr.fedorainfracloud.org/results/eclipseo/espanso/fedora-rawhide-/
type=rpm-md
skip_if_unavailable=True
gpgcheck=1
gpgkey=https://download.copr.fedorainfracloud.org/results/eclipseo/espanso/pubkey.gpg
repo_gpgcheck=0
enabled=1
enabled_metadata=1

Comment 1 Fabio Valentini 2023-09-04 10:41:11 UTC
Why are you packaging all this project's components separately? They are *not even allowed to* be packaged separately as libraries if they're not published on crates.io ...

Rust packaging macros support workspaces since version 24, so just packaging from GitHub tarball download should work. In this case, drop the "rust-" prefix from the package, and just package it as "espanso" (and use upstream tarball).

Comment 2 Fabio Valentini 2023-09-05 13:04:52 UTC
You can look at several other projects in Fedora (or pending import) that have already adopted this approach:

- firecracker
- ntpd-rs (https://github.com/decathorpe/ntpd-rs-rpms/blob/main/ntpd-rs/ntpd-rs.spec)
- system76-keyboard-configurator (does not use %cargo_generate_buildrequires with workspace support yet)

You *can* use rust2rpm for workspace projects (download sources from GitHub, extract them, and run "rust2rpm ./Cargo.toml" in the unpacked sources), but it does not automate patching Cargo.toml files.

But the way it's done right now is not correct, i.e. packaging private components / unpublished workspace members is forbidden:
https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/#_package_sources

If it helps, I can try to provide a WIP spec file for an all-in-one package.

Comment 3 Robert-André Mauchin 🐧 2023-09-21 18:34:58 UTC
Thanks for noticing,

I've got a basic issue:

+ cd espanso-2.1.8
+ cd espanso
+ /usr/bin/cargo2rpm --path Cargo.toml buildrequires --features default,wayland --with-check
Traceback (most recent call last):
  File "/usr/bin/cargo2rpm", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/usr/lib/python3.12/site-packages/cargo2rpm/__main__.py", line 111, in main
    action_buildrequires(args)
  File "/usr/lib/python3.12/site-packages/cargo2rpm/__main__.py", line 50, in action_buildrequires
    brs = workspace_buildrequires(metadata, flags, args.with_check)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/cargo2rpm/rpm.py", line 234, in workspace_buildrequires
    member_flags = metadata.get_feature_flags_for_workspace_members(flags)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/cargo2rpm/metadata.py", line 617, in get_feature_flags_for_workspace_members
    required_features[package.name].update(reqs)
    ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
KeyError: 'espanso'

I don't get why I have this error. I have it locally too when I run cargo2rpm --path Cargo.toml buildrequires. It was working correctly before.

Comment 4 Fabio Valentini 2023-09-21 19:08:52 UTC
That definitely shouldn't happen. Can you report a bug against cargo2rpm with the reproduction steps please? https://pagure.io/fedora-rust/csrgo2rpm

Comment 5 Fabio Valentini 2023-09-21 19:09:46 UTC
sorry, Typo:

https://pagure.io/fedora-rust/cargo2rpm

Comment 6 Robert-André Mauchin 🐧 2023-10-01 12:01:38 UTC
Spec URL: https://eclipseo.fedorapeople.org/for-review/espanso.spec
SRPM URL: https://eclipseo.fedorapeople.org/for-review/espanso-2.1.8-1.fc39.src.rpm

Ok, so the whole thing was rewritten with workspaces in mind:

%build
%cargo_build -- --package={espanso,espanso-clipboard,espanso-config,espanso-detect,espanso-engine,espanso-info,espanso-inject,espanso-ipc,espanso-kvs,espanso-mac-utils,espanso-match,espanso-migrate,espanso-modulo,espanso-package,espanso-path,espanso-render,espanso-ui} --bin espanso --features="default" --bin espanso-wayland --features="wayland"

%{cargo_license_summary}
%{cargo_license} > LICENSE.dependencies

%install
install -m 0755 -vd                     %{buildroot}%{_bindir}
install -m 0755 -vp target/release/espanso %{buildroot}%{_bindir}/
install -m 0755 -vp target/release/espanso-wayland %{buildroot}%{_bindir}/

All patches have been taken from the previous split packages.

Comment 7 Fabio Valentini 2023-10-01 13:25:39 UTC
Thanks! I have a few quick comments before I do a full review:

------------------------------------------------------------

The upstream tarball bundles a copy of wxWidgets. It needs to be removed in %prep:

%prep
...
# drop vendored wxWidgets sources
rm -r espanso-modulo/vendor

------------------------------------------------------------

> %cargo_build -- --package={espanso,espanso-clipboard,espanso-config,espanso-detect,espanso-engine,espanso-info,espanso-inject,espanso-ipc,espanso-kvs,espanso-mac-utils,espanso-match,espanso-migrate,espanso-modulo,espanso-package,espanso-path,espanso-render,espanso-ui} --bin espanso --features="default" --bin espanso-wayland --features="wayland"

This should not be necessary, and I also think it doesn't do what you actually want.
This should be enough to achieve the same thing:

%cargo_build -f wayland

------------------------------------------------------------

Similarly, for %cargo_test, the following should be equivalent to what you're doing manually:

%cargo_test -- -- --skip ipc_multiple_clients

------------------------------------------------------------

If you want to actually have distinct License tags for espanso and espanso-wayland, you also need to call %cargo_license / %cargo_license_summary with different arguments for both crates:

# for "default"
%{cargo_license_summary}
%{cargo_license} > LICENSE.dependencies

# for "wayland"
%{cargo_license_summary -f wayland}
%{cargo_license -f wayland} > LICENSE.dependencies.wayland

Otherwise one or the other will not be accurate.

In general, you should apply the same "-a" / "-f" flags to *all* %cargo_* macros (except %cargo_prep) to avoid inconsistent results or weird errors.

------------------------------------------------------------

Package also doesn't built yet due to missing dependencies:

- include_dir (pending review)
- log-panics (approved, pending unretirement)
- markdown (pending review)

There's also "yaml-rust-terzi", which is a fork of the "yaml-rust" crate.
The code for it will need to be bundled in this package, and the dependency replaced with a `path = "./path/to/yaml-rust"`-style dependency).


Note You need to log in before you can comment on or make changes to this bug.