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

cargo-tauri.hook: init #335751

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

Conversation

getchoo
Copy link
Member

@getchoo getchoo commented Aug 19, 2024

Description of changes

With Tauri apps getting more popular and added to nixpkgs, I believe it's time we share the tedious setup required to build them. This PR introduces a new hook in cargo-tauri.hook that builds on top of rustPlatform.buildRustPackage (or more acurately, its hooks) to leverage Tauri's bundling feature in creating packages. It is compatible with the two primary targets we share with Tauri -- Linux and macOS -- and meant to be used alongside pre-existing tools for package managers such as npm, pnpm, and yarn

Supersedes #318117

CC @doronbehar for changes made to yarnConfigHook

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.11 Release Notes (or backporting 23.11 and 24.05 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.

Result of nixpkgs-review run on x86_64-linux 1

5 packages failed to build:
  • insulator2
  • kiwitalk
  • powerdns-admin
  • surrealist
  • treedome
24 packages built:
  • auto-changelog
  • cargo-tauri
  • coc-diagnostic
  • codefresh
  • diagnostic-languageserver
  • dotenv-cli
  • get-graphql-schema
  • gitbutler
  • koodo-reader
  • kuro
  • listmonk
  • modrinth-app
  • modrinth-app-unwrapped
  • nixpkgs-manual
  • postlight-parser
  • pot
  • screego
  • spectral-language-server
  • textlint-rule-prh
  • vim-language-server
  • vimPlugins.coc-diagnostic
  • xplorer
  • yarnConfigHook
  • your_spotify

powerdns-admin has been broken since 574c80a

The remaining packages fail with the now infamous

error[E0282]: type annotations needed for Box<_>


Add a 👍 reaction to pull requests you find important.

@Eveeifyeve
Copy link

Eveeifyeve commented Aug 19, 2024

I will look into documenting this as this will help build Tauri apps on nix

@Eveeifyeve
Copy link

And soon bun I am looking to make a hook

@doronbehar
Copy link
Contributor

CC @doronbehar for changes made to yarnConfigHook

They seem good. I don't have time unfortunately to review and help with the rest of the PR :/, so I'm unsubscribing myself.

@getchoo
Copy link
Member Author

getchoo commented Aug 19, 2024

I will look into documenting this as this will help build Tauri apps on nix

I've made a section for it in the manual already here -- it should work for a vast majority of projects. There's also the test-app showing a real world example with one of the upstream templates

@happysalada
Copy link
Contributor

Editorconfig check is failing
Ofborg is also failing on insulator2

Apart from that this looks good.

@tengkuizdihar
Copy link
Contributor

because of newer rust version, I got an error when testing treedome. I can make a fix and a quick release, can the new release be in this PR?

@getchoo
Copy link
Member Author

getchoo commented Aug 19, 2024

Editorconfig check is failing

Not really sure what to do there as it's failing due to an icon file

Ofborg is also failing on insulator2

As said in the original comment, this is due to the time crate and unrelated to this PR

because of newer rust version, I got an error when testing treedome. I can make a fix and a quick release, can the new release be in this PR?

Once the update for treedome is merged into master, you can run nixpkgs-review pr 335751 --package treedome and it will automatically merge this PR locally and build treedome. I'll probably end up rebasing on master when I make more changes though, so

@tengkuizdihar tengkuizdihar mentioned this pull request Aug 19, 2024
13 tasks
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

The editorconfig CI test giving an error is the canary in the coalmine. nixpkgs should have as little vendored code as possible, and it's quite easy to make a repository for this example app.

pkgs/by-name/ca/cargo-tauri/test-app/.gitignore Outdated Show resolved Hide resolved
@Eveeifyeve
Copy link

I will look into using this once I have a bun install/config hook.

@getchoo
Copy link
Member Author

getchoo commented Aug 23, 2024

Fixed merge conflict with #335795. Treedome should build now

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I'm out of my depth on the actual content of this PR (the hook creation and use). I do like this PR and hope to learn more.

Here's a review of the content as I read through it this morning.

doc/hooks/tauri.section.md Outdated Show resolved Hide resolved
doc/hooks/tauri.section.md Show resolved Hide resolved
pkgs/by-name/ca/cargo-tauri/package.nix Show resolved Hide resolved
pkgs/by-name/ca/cargo-tauri/test-app.nix Show resolved Hide resolved
pkgs/by-name/ki/kiwitalk/package.nix Outdated Show resolved Hide resolved
# Tauri doesn't respect $CARGO_TARGET_DIR, but does respect the cargo
# argument...but that doesn't respect `--target`, so we have to use the
# config file
# https://github.com/tauri-apps/tauri/issues/10190
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, --target-dir is respected now. What I'm trying to explain here though is that when it's used alongside the target --target argument, it still tries to use target/<release type> as the target directory even though cargo will actually output to target/<target triple>/<release type>

I should probably make a patch/issue for this at some point, I just haven't had the time lol

pkgs/by-name/ca/cargo-tauri/hook.sh Outdated Show resolved Hide resolved
pkgs/by-name/ca/cargo-tauri/hook.sh Outdated Show resolved Hide resolved
pkgs/by-name/ca/cargo-tauri/hook.sh Outdated Show resolved Hide resolved
@@ -0,0 +1,125 @@
# shellcheck shell=bash
Copy link
Contributor

Choose a reason for hiding this comment

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

CC @tie for bash review (I'm slowly building my skills but they are a master)

@MDM23
Copy link
Contributor

MDM23 commented Sep 3, 2024

I'm currently updating surrealist to the latest version, which requires cargo-tauri v2. Do you see any way to have v1 and v2 as top-level packages with the new hook?

Initially, I just overwrote it locally:

https://github.com/NixOS/nixpkgs/pull/335619/files#diff-7593db6b9b5b36a086f481cfdce0d8629e002817795794ab40592d321f2e93c2R27

@nyabinary
Copy link
Contributor

nyabinary commented Sep 5, 2024

Shouldn't this be fixed as well? tauri-apps/tauri#8679

@getchoo
Copy link
Member Author

getchoo commented Sep 5, 2024

Shouldn't this be fixed as well? tauri-apps/tauri#8679

I think that would be something more for upstream, as the only way we can "fix" this is by applying the patch linked in the issue to each package. I wouldn't consider that in scope for this PR though, and probably not a good idea to do across the board as messing with vendored deps should generally be avoided. For packages that need the patch, they should be able to use it in crateOverrides similar to any other Rust package

@philiptaron
Copy link
Contributor

philiptaron commented Sep 6, 2024

Looks like #335619 sniped you. Mind rebasing?

@Eveeifyeve
Copy link

also we should make this a generic or something as there is dev version of cargo tauri cli.

@Eveeifyeve
Copy link

And we should start supporting it as it's in rc.

@getchoo
Copy link
Member Author

getchoo commented Sep 8, 2024

also we should make this a generic or something as there is dev version of cargo tauri cli.

Not sure how well that would work given the breaking changes now (and possibly in the future) in Tauri v2 from v1. I would consider these hooks to be pretty flexible though, so if there aren't that many breaking changes related to the build I doubt it would be hard to implement

And we should start supporting it as it's in rc.

That would be best in another PR

@nyabinary
Copy link
Contributor

nyabinary commented Sep 8, 2024

also we should make this a generic or something as there is dev version of cargo tauri cli.

Not sure how well that would work given the breaking changes now (and possibly in the future) in Tauri v2 from v1. I would consider these hooks to be pretty flexible though, so if there aren't that many breaking changes related to the build I doubt it would be hard to implement

And we should start supporting it as it's in rc.

That would be best in another PR

https://v2.tauri.app/blog/tauri-2-0-0-release-candidate/
https://v2.tauri.app/blog/roadmap-to-tauri-2-0/

There won't be any breaking changes in RC according to this so I think it's best to do it in this pr tbh

@getchoo
Copy link
Member Author

getchoo commented Sep 8, 2024

There won't be any breaking changes in RC

I'm talking about the breaking changes from Tauri v1 -> Tauri v2, not breaking changes from the v2 RC to stable

so I think it's best to do it in this pr tbh

Setting up the v2 packaging here initially is definitely out of scope IMO given how large this PR already is. The hooks created here will -- at the very least -- require changes to most dependencies like using GTK4 over 3, a newer webkitgtk ABI, etc. I think it would be much better to make those changes later on once we get this merged and actually package v2

@philiptaron
Copy link
Contributor

@getchoo -- please make sure to integrate the changes from #340862 when you fix the merge conflict.

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.