Skip to content

falkor: init at 0.0.92#357021

Merged
JohnRTitor merged 2 commits intoNixOS:masterfrom
IceDBorn:falkor
Jan 31, 2025
Merged

falkor: init at 0.0.92#357021
JohnRTitor merged 2 commits intoNixOS:masterfrom
IceDBorn:falkor

Conversation

@IceDBorn
Copy link
Contributor

@IceDBorn IceDBorn commented Nov 18, 2024

Falkor is an Electron-based gaming hub.

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 the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Nov 18, 2024
@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/4849

@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-already-reviewed/2617/2120

@IceDBorn
Copy link
Contributor Author

@JohnRTitor

Copy link
Member

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

Is source build not possible here?

@IceDBorn
Copy link
Contributor Author

@JohnRTitor Looking at various issues, it seems like electron-builder does not work with nix.

Copy link
Member

Choose a reason for hiding this comment

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

set hydraPlatforms = [ ]; here

Copy link
Contributor

Choose a reason for hiding this comment

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

@JohnRTitor , why was this needed? Is this here to avoid caching the package? And if yes, why?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is to put a light load on nix cache. These packages are not source builds, so it's safe to let them build locally.

This is not required persay, but good practice.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Nov 19, 2024
@TomaSajt TomaSajt self-requested a review November 20, 2024 23:28
@TomaSajt
Copy link
Contributor

There are ways to use electron-builder, though they are quite hacky.
See github search for examples
Will try to figure something out tomorrow.

@JohnRTitor
Copy link
Member

There is #279545 which could be used as an example, both are npm packages I think?

@IceDBorn
Copy link
Contributor Author

IceDBorn commented Dec 8, 2024

There is #279545 which could be used as an example, both are npm packages I think?

Nope, this one is using yarn.

@JohnRTitor
Copy link
Member

This could work, I haven't tried it though.

Yarn based projects use a `yarn.lock` file instead of a `package-lock.json` to pin dependencies. Nixpkgs provides the Nix function `fetchYarnDeps` which fetches an offline cache suitable for running `yarn install` before building the project. In addition, Nixpkgs provides the hooks:

@TomaSajt
Copy link
Contributor

TomaSajt commented Dec 8, 2024

Looked into it, and while it would be possible to package from source, the users would have to provide their own API keys for the build, which is unfortunate.

I'm guessing the prebuilt package contains the keys internally (using import.meta.env to embed them with vite)

I doubt the package authors want us to scrape those values, so I think packaging the prebuilt app is the way to go.

@IceDBorn
Copy link
Contributor Author

@TomaSajt So? Should this be merged then?

@IceDBorn IceDBorn force-pushed the falkor branch 2 times, most recently from 3da92a5 to e7175f8 Compare December 18, 2024 19:40
@IceDBorn IceDBorn requested a review from TomaSajt December 18, 2024 20:09
@JohnRTitor
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 357021

@JohnRTitor
Copy link
Member

nixpkgs-review couldn't build it for some reason, but I was able to build and run it successfully.

@JohnRTitor JohnRTitor merged commit e252f96 into NixOS:master Jan 31, 2025
@IceDBorn IceDBorn deleted the falkor branch April 11, 2025 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants