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

gitbutler: init at 0.10.11 #289664

Closed
wants to merge 16 commits into from
Closed

Conversation

hacker1024
Copy link
Member

@hacker1024 hacker1024 commented Feb 18, 2024

Description of changes

This PR adds a GitButler package that builds from source.

Unfortunately quite a lot needs to be vendored, including the Yarn lockfile (#231513), which needs to be regenerated and may differ from the upstream pnpm lockfile.

Alternative to #289131.
Closes #288567.

macOS is untested as webkitgtk has been broken for years.

If you're interested in maintaining as well, feel free to let me know or make a PR into my branch adding yourself.

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.

@hacker1024 hacker1024 marked this pull request as draft February 18, 2024 05:35
@hacker1024 hacker1024 changed the title gitbutler: iniit at 0.10.10 gitbutler: init at 0.10.10 Feb 18, 2024
@hacker1024 hacker1024 marked this pull request as ready for review February 18, 2024 06:49
Copy link
Contributor

@Stunkymonkey Stunkymonkey left a comment

Choose a reason for hiding this comment

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

LGTM

pkgs/by-name/gi/gitbutler-ui/package.nix Outdated Show resolved Hide resolved
Copy link
Member

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

Works quite well 👍 Better than the deb-based package (#289131).

That said, I'm a bit unsure about the bin/gitbutler-app, where-as the released version is named bin/git-butler. I couldn't figure out where in the release process this new name is applied, but it might be a good idea to align the Nix output with the released naming. Renaming in fixup could be enough if there is no proper way to do it.

@bobvanderlinden bobvanderlinden mentioned this pull request Feb 18, 2024
13 tasks

meta = gitbutler-ui.meta // {
description = "A Git client for simultaneous branches on top of your existing workflow.";
platforms = with lib.platforms; linux ++ darwin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that an actual limitation of the program, or does it work on any platform which Rust can target (e.g: BSD)?

If the latter, don't set meta.platforms as buildRustPackage already sets it to the correct value.

Copy link
Member Author

@hacker1024 hacker1024 Feb 20, 2024

Choose a reason for hiding this comment

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

Upstream only claims to support Linux and macOS, so I marked it as such. Is there a precedent for doing otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platforms = with lib.platforms; linux ++ darwin;

buildRustPackage already defaults to this

@Stunkymonkey
Copy link
Contributor

Result of nixpkgs-review pr 289664 run on x86_64-linux 1

2 packages built:
  • gitbutler
  • gitbutler-ui

1 similar comment
@geri1701
Copy link
Member

Result of nixpkgs-review pr 289664 run on x86_64-linux 1

2 packages built:
  • gitbutler
  • gitbutler-ui

@ambroisie
Copy link
Contributor

@getchoo just took a look, using a FOD which calls pnpm directly is not the way to go about packaging pnpm dependencies, and until #231513 is fixed, we have to get around that issue by using yarn or npm packaging.

@getchoo
Copy link
Member

getchoo commented Feb 22, 2024

@getchoo just took a look, using a FOD which calls pnpm directly is not the way to go about packaging pnpm dependencies

why not?

and until #231513 is fixed, we have to get around that issue by using yarn or npm packaging.

why? there is already precedent for this in other packages, with the former even being another tauri package using it for the same purpose. i genuinely don't see a reason as to why this shouldn't be used, or how not having standard tooling should force us into dealing with the pretty big maintenance burden (and error prone process) of keeping a lockfile at parity with upstream.

if there's a more robust solution that doesn't drastically increase maintenance, i'd love to see it; but having our own lockfile from a different package manager is not it.

@ambroisie
Copy link
Contributor

why not?

@getchoo for the exact reason explained in the linked issue. If and when pnpm changes their store format, suddenly every FOD derivation based on it is invalid and won't build anymore.

there is already precedent for this in other packages

This package shouldn't have been approved and merged in that state.

how not having standard tooling should force us into dealing with the pretty big maintenance burden (and error prone process) of keeping a lockfile at parity with upstream.

Because when pnpm is updated and breaks our FODs, we have more churn to go through. Whereas creating lock files can easily be automated through update scripts if needed.

@getchoo
Copy link
Member

getchoo commented Feb 22, 2024

If and when pnpm changes their store format

this isn't often from my experience with vesktop; seems to be working well in pot as well.
i'm not trying to say this isn't an issue at all, but it's not one big enough to go and maintain our own separate lockfile

This package shouldn't have been approved and merged in that state.

this is very subjective

Because when pnpm is updated and breaks our FODs, we have more churn to go through. Whereas creating lock files can easily be automated through update scripts if needed.

i would argue the churn required to update the hash is less than or equal to - as the comment in says in the current revision - "[the work required] to test the result thoroughly as dependency versions may differ from the release". we shouldn't be sacrificing the use of upstream's tested set of dependencies for marginal at best productivity gains through only some automation (that hasn't even been implemented)

couldn't we also update the FOD hash through an update script?

@getchoo getchoo mentioned this pull request Feb 22, 2024
16 tasks
@hacker1024
Copy link
Member Author

I do not think there's much point in discussing the pros and cons of the pnpm technique here - that's why we have #231513 and #290715.

Until those discussions are resolved, it does not seem appropriate to copy the contested process once again. I'd prefer to stick with the tried and true Yarn method, despite potential problems arising due to mismatched lockfiles.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3574

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Please squash all the reeview comment for each package into the init commit.

distPhase = "true";

meta = rec {
description = "The UI for GitButler.";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description = "The UI for GitButler.";
description = "Git client for simultaneous branches on top of your existing workflow";

license = lib.licenses.fsl-10-mit;
maintainers = with lib.maintainers; [ hacker1024 ];
platforms = with lib.platforms; all;
sourceProvenance = with lib.sourceTypes; [ fromSource ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sourceProvenance = with lib.sourceTypes; [ fromSource ];

That doesn't need to be set

changelog = "https://github.com/gitbutlerapp/gitbutler/releases/tag/release/${version}";
license = lib.licenses.fsl-10-mit;
maintainers = with lib.maintainers; [ hacker1024 ];
platforms = with lib.platforms; all;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platforms = with lib.platforms; all;
platforms = lib.platforms.all;

meta = rec {
description = "The UI for GitButler.";
homepage = "https://gitbutler.com";
downloadPage = homepage;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
downloadPage = homepage;

there is no point in repeating this.
Also this is used wrongly, since it is not a direct link to the download page

Comment on lines +19 to +22
let
version = "0.10.11";
in
assert lib.assertMsg (version == gitbutler-ui.version) "The GitButler version does not match the GitButler UI version!";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let
version = "0.10.11";
in
assert lib.assertMsg (version == gitbutler-ui.version) "The GitButler version does not match the GitButler UI version!";

if we inherit the version anyway, then there is not really a point doing this

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 it might be useful to prevent improper overrides. But the current version will not do that, as you point out.

'';

postInstall = ''
mv "$out"/bin/{gitbutler-app,'${meta.mainProgram}'}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mv "$out"/bin/{gitbutler-app,'${meta.mainProgram}'}
mv "$out"/bin/{gitbutler-app,'git-butler'}

a change to meta should not trigger a rebuild

genericName = "Git client";
categories = [ "Development" ];
comment = meta.description;
exec = meta.mainProgram;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
exec = meta.mainProgram;
exec = "git-butler";

desktopName = "GitButler";
genericName = "Git client";
categories = [ "Development" ];
comment = meta.description;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
comment = meta.description;
comment = "Git client for simultaneous branches";

most DEs don't render so long comments that great


meta = gitbutler-ui.meta // {
description = "A Git client for simultaneous branches on top of your existing workflow.";
platforms = with lib.platforms; linux ++ darwin;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platforms = with lib.platforms; linux ++ darwin;

buildRustPackage already defaults to this

];

meta = gitbutler-ui.meta // {
description = "A Git client for simultaneous branches on top of your existing workflow.";
Copy link
Member

Choose a reason for hiding this comment

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

With my other review comment the description gets already set to this plus it will be compliant with the contributing guide which states that no A in the front or dot at the end should be.

Copy link
Member

Choose a reason for hiding this comment

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

Where is "no A in the front" stated?

https://github.com/NixOS/nixpkgs/tree/master/pkgs#meta-attributes doesn't.

https://nixos.org/manual/nixpkgs/unstable/#var-meta-description even puts a right example with this.

I don't think the A at the beginning caused any problems. In fact, in most cases it even makes the description read more fluently.

@hallettj
Copy link

hallettj commented Mar 19, 2024

How did you generate yarn.lock? I'm attempting to update the package for 0.10.24, but it seems I used a different method, and the yarn.lock diff is very large. Also I'm not sure my yarn.lock works correctly. What I tried was:

nix run nixpkgs#pnpm-lock-export -- --schema yarn.lock@v1

Edit: Oh I'm sorry - I finally saw the comment:

To generate the Yarn lockfile, run yarn install.

Also while I'm here, this package does not run on Wayland without XWayland. (Gitbutler's appimage has the same issue.) There is an error, "(git-butler:2957473): Gtk-WARNING **: 11:53:42.101: cannot open display: ". But from what I've read it seems like Tauri apps should run in Wayland? Anyone happen to know if there is a cargo feature that needs to be enabled, or an env var that needs to be set or something?

Edit: Updating gitbutler to 0.10.24 fixed the Wayland issue for me.

Thanks for working on the package!

@ambroisie
Copy link
Contributor

nix run nixpkgs#pnpm-lock-export -- --schema yarn.lock@v1

Edit: Oh I'm sorry - I finally saw the comment:

To generate the Yarn lockfile, run yarn install.

If pnpm-lock-export works on this package, I'd recommend using it as it would allow us to use the same version of the packages as upstream specified.

@hallettj
Copy link

hallettj commented Mar 23, 2024

If pnpm-lock-export works on this package, I'd recommend using it as it would allow us to use the same version of the packages as upstream specified.

You're right, and it's probably important to match versions especially with the dependencies fetched via git. Unfortunately pnpm-lock-export is not handling git dependencies correctly when I try it. For example when I use yarn install I get this in yarn.lock:

"tauri-plugin-log-api@github:tauri-apps/tauri-plugin-log#v1":
  version "0.0.0"
  resolved "https://codeload.github.com/tauri-apps/tauri-plugin-log/tar.gz/db7255ca2e07fc4d3e6cc5d93f9ccfceacb28901"
  dependencies:
    "@tauri-apps/api" "1.5.3"

When I use pnpm-lock-export I get this instead:

"tauri-plugin-log-api@0.0.0":
  version "0.0.0"
  resolved "https://codeload.github.com/tauri-apps/tauri-plugin-log/tar.gz/19f5dcc0425e9127d2c591780e5047b83e77a7c2"
  integrity "undefined"
  dependencies:
    "@tauri-apps/api" "1.5.3"

Note that the git hash in the second matches the hash in the original lock file.

The pnpm-lock-export version leads to a build error. I can fix it by doing some fix-up by hand: deleting the integrity "undefined" line, and changing the specifier (the part after the @ on the first line). But then I'm getting an error saying that an esbuild package can't be found. I make experiment some more.

gitbutler-ui-modules> yarn config v1.22.19
gitbutler-ui-modules> success Set "yarn-offline-mirror" to "/nix/store/w8rw695xiggyqj1j46lc7v1iz77fh1a9-offline".
gitbutler-ui-modules> Done in 0.01s.
gitbutler-ui-modules> yarn install v1.22.19
gitbutler-ui-modules> [1/4] Resolving packages...
gitbutler-ui-modules> error Can't make a request in offline mode ("https://github.com/tauri-apps/tauri-plugin-log")
gitbutler-ui-modules> info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
gitbutler-ui-modules> Error: Can't make a request in offline mode ("https://github.com/tauri-apps/tauri-plugin-store")
gitbutler-ui-modules>     at MessageError.ExtendableBuiltin (/nix/store/f8nf1q0lffdni21lfw9frd5f9z231nzp-yarn-1.22.19/libexec/yarn/lib/cli.js:721:66)
gitbutler-ui-modules>     at new MessageError (/nix/store/f8nf1q0lffdni21lfw9frd5f9z231nzp-yarn-1.22.19/libexec/yarn/lib/cli.js:750:123)
gitbutler-ui-modules>     at RequestManager.request (/nix/store/f8nf1q0lffdni21lfw9frd5f9z231nzp-yarn-1.22.19/libexec/yarn/lib/cli.js:65887:59)
gitbutler-ui-modules>     at GitHubResolver.<anonymous> (/nix/store/f8nf1q0lffdni21lfw9frd5f9z231nzp-yarn-1.22.19/libexec/yarn/lib/cli.js:33912:48)
gitbutler-ui-modules>     at Generator.next (<anonymous>)
gitbutler-ui-modules>     at step (/nix/store/f8nf1q0lffdni21lfw9frd5f9z231nzp-yarn-1.22.19/libexec/yarn/lib/cli.js:310:30)
gitbutler-ui-modules>     at /nix/store/f8nf1q0lffdni21lfw9frd5f9z231nzp-yarn-1.22.19/libexec/yarn/lib/cli.js:328:14
gitbutler-ui-modules>     at new Promise (<anonymous>)
gitbutler-ui-modules>     at new F (/nix/store/f8nf1q0lffdni21lfw9frd5f9z231nzp-yarn-1.22.19/libexec/yarn/lib/cli.js:5305:28)
gitbutler-ui-modules>     at GitHubResolver.<anonymous> (/nix/store/f8nf1q0lffdni21lfw9frd5f9z231nzp-yarn-1.22.19/libexec/yarn/lib/cli.js:307:12)

Edit: I found out the esbuild problem comes up because pnpm-lock-export does not copy optionalDependencies to the generated yarn.lock

@hallettj
Copy link

It turns out that pnpm-lock-export hasn't been updated in quite a while. I dusted it off, updated it for the latest pnpm-lock.yaml format, and got it into good shape for converting gitbutler's lock file to a yarn.lock. (I didn't do much to update conversions to package-lock.json - that might need some more work.)

I updated my PR off of this PR, hacker1024#2, to use the converted lock file.

My pnpm-lock-export branch is here: https://github.com/hallettj/pnpm-lock-export

hallettj added a commit to hallettj/home.nix that referenced this pull request Mar 25, 2024
There is a nixpkgs package in progress at NixOS/nixpkgs#289664
@ambroisie
Copy link
Contributor

@hallettj the current packaged version is a fork by @adamcstephens to support the v6 lock format, feel free to open a PR to update the package if it improves on it in any way.

@hacker1024
Copy link
Member Author

Sorry for the long silence, I'll update this PR shortly. Thanks everyone!

@luftmensch-luftmensch
Copy link
Contributor

Any updates on this PR?

@xyven1
Copy link
Contributor

xyven1 commented May 12, 2024

Bumping this.

@Aleksanaa
Copy link
Member

Hi, @DataHearth has opened a new pr #289664. Do you still want to work on this or prefer their PR instead?

@DataHearth
Copy link
Contributor

Hi, @DataHearth has opened a new pr #289664. Do you still want to work on this or prefer their PR instead?

Hi ! The only prerequisite I need for my PR is someone with a MacOS system to validate the unpack phase. If the current way is not valide, I already have the backup to make it works.

For linux, I'm using the appimage. I didn't see any runtime dependencies required for now that wasn't bundled with the appimage (Like jdk for nosql-workbench I've already bundled).

I've already tested and used it on x64 linux.

Cheers!

@luftmensch-luftmensch
Copy link
Contributor

Hi, @DataHearth has opened a new pr #289664. Do you still want to work on this or prefer their PR instead?

It hasn't been decided to use the source based version instead of the AppImage?

@alyssais
Copy link
Member

Generally in Nixpkgs we prefer to build from source where possible.

@luftmensch-luftmensch
Copy link
Contributor

Generally in Nixpkgs we prefer to build from source where possible.

So #289664 should be preferred, and hopefully closed shortly.

@msanft msanft mentioned this pull request May 14, 2024
13 tasks
Comment on lines +20 to +30
# The package.json must use spaces instead of upstream's tabs to pass Nixpkgs
# CI.
# To generate the Yarn lockfile, run `yarn install`.
# There is no way to import the tagged pnpm lockfile, so make sure to test the
# result thoughly as dependency versions may differ from the release.
packageJSON = ./package.json;
yarnLock = ./yarn.lock;
offlineCache = fetchYarnDeps {
inherit yarnLock;
hash = "sha256-rggtkfE6An8It0Rvgfk0J8JHpg0NbLiweRsz0nM/tzM=";
};
Copy link
Member

Choose a reason for hiding this comment

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

#290715 👀

@getchoo getchoo mentioned this pull request Jun 7, 2024
13 tasks
@hacker1024
Copy link
Member Author

Closing in favour of #318101.

@hacker1024 hacker1024 closed this Jun 9, 2024
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.

Package request: GitButler