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

espanso: 0.7.3 -> 2.1.8 #208949

Closed
wants to merge 1 commit into from
Closed

Conversation

pyrox0
Copy link
Member

@pyrox0 pyrox0 commented Jan 3, 2023

Description of changes

This is a major update of espanso, with breaking(but fixable) changes.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • [] (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation labels Jan 3, 2023
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 labels Jan 3, 2023
@dit7ya
Copy link
Member

dit7ya commented Jan 4, 2023

Can you please write about how to test this package? Is this supposed to work on Wayland as well?

@pyrox0
Copy link
Member Author

pyrox0 commented Jan 4, 2023

Can you please write about how to test this package? Is this supposed to work on Wayland as well?

It is supposed to work on wayland. If you use flakes, I would suggest pulling the branch in as an input and making an overlay or just pulling the package out of the set. Then you could have a small config in your $CONFIG/espanso folder, which would be picked up by the service. This should also be able to build on MacOS, though I have not been able to test it.

@pyrox0 pyrox0 force-pushed the espanso-update-2.1.8 branch 2 times, most recently from 5a552ca to 58f0450 Compare January 4, 2023 20:07
@NickCao
Copy link
Member

NickCao commented Jan 17, 2023

As part of #210452, would you like to also remove the openssl_1_1 pin in all-packages.nix?

@pyrox0 pyrox0 force-pushed the espanso-update-2.1.8 branch 2 times, most recently from 6ae3aa0 to 7a6265f Compare January 17, 2023 03:27
@pyrox0
Copy link
Member Author

pyrox0 commented Jan 17, 2023

Ok, I'm currently running into some problems where cargo-make(the tool used by espanso for building, which allows enabling Wayland support) needs to fetch a dependency at build-time, but network access is disabled and we've already fetched deps by the time that step happens. Any ideas on how to fix this?

This is the log I have from the build
espanso.log

@NickCao
Copy link
Member

NickCao commented Jan 18, 2023

Would adding rust-script to nativeBuildInputs help?

@PedroHLC
Copy link
Member

@theHedgehog0 any update on this?

postInstall = ''
wrapProgram $out/bin/espanso \
--prefix PATH : ${lib.makeBinPath [ libnotify xclip ]}
--prefix PATH : ${lib.makeBinPath [ libnotify xclip wl-clipboard setxkbmap ]}
Copy link

@hugosenari hugosenari Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but I think you should verify waylandSupport for wl-clipboard
And !waylandSupport for xclip

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think when waylandSupport is enabled, it should still be compatible with X11? So that means xclip should always be available? (not sure, correct me if i'm wrong)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not wayland user, maybe @AndersonTorres knows.

];

NO_X11 = lib.optionals waylandSupport "true";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be

Suggested change
NO_X11 = lib.optionals waylandSupport "true";
env.NO_X11 = lib.boolToString waylandSupport;

?


nativeBuildInputs = [
extra-cmake-modules
pkg-config
makeWrapper
cargo-make
wxGTK32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a nativeBuildInput?

Copy link
Member

@bobvanderlinden bobvanderlinden May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried building without wxGTK32 as a nativeBuildInput, but the build failed.

       > error: failed to run custom build command for `espanso-modulo v0.1.0 (/build/source/espanso-modulo)`
       >
       > Caused by:
       >   process didn't exit successfully: `/build/source/target/release/build/espanso-modulo-8653a240257a137b/build-script-build` (exit status: 101)
       >   --- stderr
       >   thread 'main' panicked at 'wxWidgets is not installed, as `wx-config` cannot be executed', espanso-modulo/build.rs:467:5
       >   note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
       > warning: build failed, waiting for other jobs to finish...
       For full logs, run 'nix log /nix/store/dnn037vxl868icgfyimfwfcb604x1zfy-espanso-2.1.8.drv'.

That said, removing it from buildInputs still worked. I'm unsure why it only works in nativeBuildInputs

@bobvanderlinden
Copy link
Member

It seems espanso-0.7.3 is currently broken on nixpkgs-unstable.

       > error[E0793]: reference to packed field is unaligned
       >    --> /build/espanso-0.7.3-vendor.tar.gz/tendril/src/tendril.rs:241:20
       >     |
       > 241 |                 if (*header).refcount.decrement() == 1 {
       >     |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       >     |
       >     = note: fields of packed structs are not properly aligned, and creating a misaligned reference is undefined behavior (even if that reference is never dereferenced)
       >     = help: copy the field contents to a local variable, or replace the reference with a raw pointer and use `read_unaligned`/`write_unaligned` (loads and stores via `*p` must be properly aligned even when using raw pointers)
       >
       > error[E0793]: reference to packed field is unaligned
       >    --> /build/espanso-0.7.3-vendor.tar.gz/tendril/src/tendril.rs:958:9
       >     |
       > 958 |         (*self.header()).refcount.increment();
       >     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       >     |
       >     = note: fields of packed structs are not properly aligned, and creating a misaligned reference is undefined behavior (even if that reference is never dereferenced)
       >     = help: copy the field contents to a local variable, or replace the reference with a raw pointer and use `read_unaligned`/`write_unaligned` (loads and stores via `*p` must be properly aligned even when using raw pointers)
       >
       > For more information about this error, try `rustc --explain E0793`.
       > error: could not compile `tendril` due to 2 previous errors
       > warning: build failed, waiting for other jobs to finish...
       For full logs, run 'nix log /nix/store/99jxamy1ap48s6ispss4pxzq0zi29w05-espanso-0.7.3.drv'.

This branch works perfectly for me. This is what I did to test it. Note that I don't have much experience with espanso, as this was the first time I wanted to give it a try (but got stuck due to the build errors in 0.7.3).

$ nix shell .#espanso
$ espanso --version
espanso 2.1.8
$ espanso service start --unmanaged
$ espanso cmd search

I was able to add matches to ~/.config/espanso/match/base.yml, which showed up in the search and expanded in text fields.

This is all under X11. I have not tested this on Wayland. waylandSupport = false or waylandSupport = true both seemed to work for me.

I've rebased the branch on nixpkgs-unstable and incorporated the suggestions. You can find the branch here: https://github.com/bobvanderlinden/nixpkgs/tree/espanso-update-2.1.8

@bobvanderlinden
Copy link
Member

Espanso 0.7.3 is failing and it is part of #230712.

The changes in this PR are at least an improvement over a failed build, so I'd vouch for merging.

https://github.com/bobvanderlinden/nixpkgs/tree/espanso-update-2.1.8 handles a number of the feedback. Even if the package hasn't been fully worked out with all conventions, it at least fixes the problem of a failing build on master.

@AndersonTorres
Copy link
Member

How can this be merged?

Screenshot_20230509-175613_GitHub_1.png

@bobvanderlinden
Copy link
Member

bobvanderlinden commented May 9, 2023

How can this be merged?

The conflicts are trivial to solve. See bobvanderlinden@8ca5e19 for an up-to-date commit of this pr.

Edit: alternatively I can create a new PR with those changes.

@AndersonTorres
Copy link
Member

Please do it, preferably linking this PR too.

@pyrox0
Copy link
Member Author

pyrox0 commented May 9, 2023

Hey, very sorry about not updating this in a while, I've been very busy, and FOSS just can't be my priority at the moment. I'm glad someone else is working on this, but I'll close this. I'd still like to be added to the maintainers list for the package, if that would be possible!

@pyrox0 pyrox0 closed this May 9, 2023
@bobvanderlinden bobvanderlinden mentioned this pull request May 10, 2023
12 tasks
@bobvanderlinden
Copy link
Member

Not a problem! I've created a new one #231062 and kept most of your commit intact. The feedback is incorporated and I've rebase it on nixpkgs-unstable and resolved the conflicts / incompatibilities that popped up (Cargo.lock needed to be added to the PR).

Anyone up for a review?

@pyrox0 pyrox0 deleted the espanso-update-2.1.8 branch April 24, 2024 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants