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

modrinth-app-unwrapped: refactor and use pnpm hooks #341085

Closed
wants to merge 2 commits into from

Conversation

TomaSajt
Copy link
Contributor

Description of changes

pnpm_*.configHook was added some time ago, and this package wasn't yet using it, so I refactored the package to use it.

I also simplified the way it's decided which bundle to generate. I can revert this, if needed.

In another commit (that I will merge into the first one, if decided that it's an improvement) I made the package use mkDerivation combined with cargoSetupHook, since that's the only functionality that the package was using anyway (because the build phase is overridden).
This allowed getting rid of the rec added in the first commit.


Note: the theseus repo has been renamed to code and combined into a monorepo. I didn't update the repo of fetchFromGitHub yet, just wanted to share this.
It seems that upstream is only keeping the tag for the latest version of the package in the code repo. Might need to ask them to start tagging properly.

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.

Add a 👍 reaction to pull requests you find important.

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

The new pnpm hooks have already been adopted in #336494 along with some other fixes/improvements. Regarding the other refactors here

  • I'm not sure why we would switch to mkDerivation

    • The only real benefit is with improved overriding, but I don't think that's worth the cost of us reconstructing parts of buildRustPackage
    • It will be mostly moot after cargo-tauri.hook: init #335751, which is meant to work alongside rustPlatform to easily control build profiles, flags, cross compilation, etc.
  • The way bundles are generated is a lot more naive in regards to platform support

    • Currently it's much easier to add more platforms in an attrset rather than constantly adding on to an if .. else if chain
    • Will also be moot after cargo-tauri.hook: init #335751

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Sep 12, 2024

Regarding the other refactors here

* I'm not sure why we would switch to `mkDerivation`
  
  * The only real benefit is with improved overriding, but I don't think that's worth the cost of us reconstructing parts of `buildRustPackage`
  * It will be mostly moot after [cargo-tauri.hook: init #335751](https://github.com/NixOS/nixpkgs/pull/335751), which is meant to work alongside `rustPlatform` to easily control build profiles, flags, cross compilation, etc.

* The way bundles are generated is a lot more naive in regards to platform support
  
  * Currently it's much easier to add more platforms in an attrset rather than constantly adding on to an `if .. else if` chain
  * Will also be moot after [cargo-tauri.hook: init #335751](https://github.com/NixOS/nixpkgs/pull/335751)

Ah, thanks, didn't know about that PR. Great to hear that it exists.
I'll probably close this PR then.

@TomaSajt
Copy link
Contributor Author

The new pnpm hooks have already been adopted in #336494 along with some other fixes/improvements.

Ah, I forgot to search for existing PR, I should remember to do that more often.

@TomaSajt TomaSajt closed this Sep 12, 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.

2 participants