Skip to content

ytmdesktop: build from source#397639

Merged
TomaSajt merged 1 commit intoNixOS:masterfrom
TomaSajt:ytmdesktop
Apr 28, 2025
Merged

ytmdesktop: build from source#397639
TomaSajt merged 1 commit intoNixOS:masterfrom
TomaSajt:ytmdesktop

Conversation

@TomaSajt
Copy link
Contributor

@TomaSajt TomaSajt commented Apr 10, 2025

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 10, 2025
@TomaSajt TomaSajt force-pushed the ytmdesktop branch 4 times, most recently from e3c2880 to 84ee485 Compare April 10, 2025 11:14
@TomaSajt TomaSajt marked this pull request as ready for review April 10, 2025 11:14
@nix-owners nix-owners bot requested a review from cjshearer April 10, 2025 11:23
Copy link
Member

@cjshearer cjshearer left a comment

Choose a reason for hiding this comment

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

LGTM - glad to see building packages with yarn-berry got better over this past year.

@TomaSajt
Copy link
Contributor Author

Well, I don't know what it was like before, but as you may have seen, we aren't getting the same hash on linux and darwin...

@cjshearer
Copy link
Member

cjshearer commented Apr 10, 2025

Could it be some platform dependent download? What if you specify env.ELECTRON_SKIP_BINARY_DOWNLOAD = "1";, like in this package.

It seems to be common for electron packages that use yarn.

@TomaSajt
Copy link
Contributor Author

I'm 99% sure that won't help, that env var is only used when running electron's rebuild script


We could maybe just keep the dual hash like this, and once it gets cached by hydra, we can easily compare the two.

@cjshearer
Copy link
Member

I don't think it's a big deal. If you want, you could see if env.CI = 1; affects the installation behavior in any platform-dependent way.

I recall there being a lot of things I had to disable when I first tried getting this to build on nix from source.

flokli
flokli previously requested changes Apr 24, 2025
Copy link
Member

@flokli flokli left a comment

Choose a reason for hiding this comment

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

There now is a yarn-berry fetcher, see #399404.

@TomaSajt
Copy link
Contributor Author

@yuyuyureka could you help with figuring out why the config hook is failing?

@yuyuyureka
Copy link
Contributor

The fix for that problem is now included in #401631

Can I push a version that works on your branch?

@TomaSajt
Copy link
Contributor Author

I see, you added the dependency that would have been installed with npx.

I think the better solution would be to just patch that script out.
Or, since it doesn't seem like any of them are required, just disabling all scripts would work fine too.

@github-actions github-actions bot removed 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Apr 25, 2025
@yuyuyureka
Copy link
Contributor

yuyuyureka commented Apr 25, 2025

A short explanation for the changes I pushed:

The dependency fastify-socket.io@npm:5.1.0 runs a npx only-allow [...] in its postInstall script. This is evil, because only-allow is not specified as a dependency of fastify-socket.io, so it is not present in the cache and npx will go and download the latest version of only-allow, which is obviously not reproducible.

So I added a patch to the package.json and yarn.lock to add only-allow as a dependency, which means npx will use the existing installed version at that point.
We should consider making an upstream PR to add only-allow to the dependencies in https://github.com/ducktors/fastify-socket.io/.

Next, I ran into the issue that the only-allow script would not run, because it has a shebang with #!/usr/bin/env which does not exist in the sandbox. I split the "yarn install" in yarnBerryConfigHook in two parts, and added a patchShebangs in between, similar to how npmConfigHook does it.

I did not consider skipping the build scripts entirely, because I don't know if that impacts the functionality of the app now or in the future. This might be an option for you if you know the application well and can confirm that it works without them.

@TomaSajt
Copy link
Contributor Author

Thanks for figuring this out :)

I think I will go with the YARN_ENABLE_SCRIPTS solution for now.
If you think the patchShebangs modification is still a change worth making, please do it in another PR.

@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Apr 25, 2025
@teutat3s
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 397639


x86_64-linux

✅ 1 package built:
  • ytmdesktop

@TomaSajt
Copy link
Contributor Author

Currently broken on Darwin, don't merge yet.

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Apr 26, 2025

Whhyyyyy.....
It's NixOS/nix#12698

I opened #303307

@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. 12.approvals: 3+ This PR was reviewed and approved by three or more persons. labels Apr 26, 2025
@TomaSajt
Copy link
Contributor Author

It builds fine on ofborg's darwin, because it doesn't have the sandbox enabled.
Will merge soon, if there are no more objections.

@TomaSajt TomaSajt merged commit 62dc652 into NixOS:master Apr 28, 2025
36 of 37 checks passed
@TomaSajt TomaSajt deleted the ytmdesktop branch June 19, 2025 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants