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

feishin: 0.5.1 -> 0.6.1, build from source #303638

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

jlbribeiro
Copy link
Member

@jlbribeiro jlbribeiro commented Apr 12, 2024

Description of changes

Addresses #296939 by building Feishin from source. /cc @YoshiRulz
Fixes #287765 (package update request). /cc @nadir-ishiguro
Addresses #295770 (outdated Electron). /cc @yu-re-ka
Should also fix #262739 (build failure). /cc @lunik1

This PR continues the work started by @dotlambda in #288210.
(@dotlambda, I hope you don't mind I've added you as a Co-authored-by of the first commit; can remove if necessary.)

Couldn't test this on darwin, so I'm unsure how the desktop entry plays out in MacOS systems.

Maintainer: @onny

(while it is mostly ready to review, I did accidentally press the "ready to review" button, since I assumed I'd be force-pushing some changes after review, maybe. speaking of which... I sincerely apologize for the absolute spam in all the mentioned issues and pull-requests...)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@jlbribeiro jlbribeiro marked this pull request as ready for review April 12, 2024 14:53
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Apr 12, 2024
jlbribeiro and others added 2 commits April 12, 2024 16:11
Addresses NixOS#296939 (build from source).
Continues the work started by @dotlambda in NixOS#288210.

Co-authored-by: Robert Schütz <[email protected]>
Diff: jeffvli/feishin@v0.5.1...v0.6.1
Changelog: https://github.com/jeffvli/feishin/releases/tag/v0.6.1

Feishin now depends on electron_27; electron_25 has been marked
as EOL since 9652f98.

Fixes NixOS#287765 (package update request) and addresses NixOS#295770 (outdated Electron).
@nadir-ishiguro
Copy link
Contributor

Well, this is great. Builds and runs fine on my system:

nix-shell -p nix-info --run "nix-info -m"                         
 - system: `"x86_64-linux"`
 - host os: `Linux 6.8.5-cachyos, NixOS, 24.05 (Uakari), 24.05.20240410.1042fd8`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.18.2`
 - nixpkgs: `/etc/nix/path/nixpkgs`

@yu-re-ka yu-re-ka merged commit 623ac95 into NixOS:master Apr 12, 2024
26 of 28 checks passed
@lunik1
Copy link
Contributor

lunik1 commented Apr 12, 2024

Stellar work. Builds and runs fine for me on linux.

Unfortunately I have a build failure on my M1 mac. Log is here.

@jlbribeiro jlbribeiro deleted the feishin-0.6.1 branch April 12, 2024 20:52
@jlbribeiro
Copy link
Member Author

jlbribeiro commented Apr 12, 2024

@lunik1 First of all, thank you for having provided your NUR feishin package (which I noticed just now is marked as x86_64-linux only 😛) to the community; I never got to use it because I focused on getting this to work out of stubbornness, but it was nice there was/is an alternative. (It's actually how I found out about NUR in the first place!)

Thank you so much for having tested this package on darwin; as I mentioned on the PR description, I couldn't test it on darwin, so I apologize for having broken it. I'll probably need to get a VM with MacOS working for testing this.

A quick NixOS/nixpkgs search based on "corrupted Electron dist" (from your logs) lead me to this issue/comment, which seems to point in the right direction.

@lunik1
Copy link
Contributor

lunik1 commented Apr 12, 2024

No worries! Thanks for actually putting in the work and getting the source build working! It not working on darwin isn't a huge issue for me since that's my work machine (hence why my NUR package was x86_64 linux only), but I just wanted to help out by checking the build since you couldn't.

@jlbribeiro jlbribeiro mentioned this pull request Apr 19, 2024
13 tasks
wegank pushed a commit to jlbribeiro/nixpkgs that referenced this pull request Apr 30, 2024
wegank pushed a commit to jlbribeiro/nixpkgs that referenced this pull request Apr 30, 2024
Follows NixOS#303638 and feishin darwin builds' fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update request: feishin 0.5.1 → 0.5.3 Build failure: feishin
5 participants