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

awakened-poe-trade: init at 3.25.102 #341252

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nezia1
Copy link
Contributor

@nezia1 nezia1 commented Sep 11, 2024

Description of changes

I've added Awakened POE (a Path of Exile trading app for price checking).

I ended up packaging it for Nix using the AppImage, as the build process for the app involves using a build script from upstream that uses import electron, which would use the Node.JS electron version and not take advantage of NixOS' dependency system. Upstream changes would likely need to be made to be packaged from source, or there is probably something I don't know (this is one of my first PRs to nixpkgs).

Closes #302672.

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.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Sep 11, 2024
@nezia1 nezia1 marked this pull request as draft September 11, 2024 21:10
@nezia1 nezia1 changed the title Package awakened poe (closes #302672) awakened-poe: init at 3.25.102 Sep 11, 2024
@nezia1 nezia1 marked this pull request as ready for review September 11, 2024 21:14
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

meta.changelog and meta.sourceProvenance missing

pkgs/by-name/aw/awakened-poe/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/aw/awakened-poe/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/aw/awakened-poe/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/aw/awakened-poe/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/aw/awakened-poe/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/aw/awakened-poe/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/aw/awakened-poe/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/aw/awakened-poe/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/aw/awakened-poe/package.nix Outdated Show resolved Hide resolved
@nezia1 nezia1 force-pushed the package-awakened-poe branch from d0ee864 to 482e98e Compare September 11, 2024 21:45
@nezia1
Copy link
Contributor Author

nezia1 commented Sep 11, 2024

Renamed the package to go under pkgs/by-name/aw/awakened-poe-trade, and renamed all executables and desktop files accordingly. I think you're right and we should keep it consistent with other maintainers. Also added the meta you told me to add (force pushed and squished commits).

@dotlambda
Copy link
Member

Commit message and PR title are wrong.

@nezia1 nezia1 changed the title awakened-poe: init at 3.25.102 awakened-poe-trade: init at 3.25.102 Sep 11, 2024
@nezia1 nezia1 force-pushed the package-awakened-poe branch from 482e98e to e98e53f Compare September 11, 2024 21:53
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

description = "Path of Exile trading app for price checking";
homepage = "https://github.com/SnosMe/awakened-poe-trade";
changelog = "https://github.com/SnosMe/awakened-poe-trade/releases/tag/v${version}";
sourceProvenance = [ lib.sourceTypes.binaryBytecode ];
Copy link
Member

Choose a reason for hiding this comment

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

What about it is bytecode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure which option to use. Also, thank you for pointing out that I use pname way too much, I'll change that as well. Should anything else be changed in the code I committed, or would it be good after both these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side-note: should I ignore this warning?

❯ ./result/bin/awakened-poe-trade
hook_thread_proc [101]: Could not set thread priority 49 for thread 0x7F0BBEC7F6C0!
load_input_helper [1827]: XkbGetKeyboard failed to locate a valid keyboard!
APPIMAGE env is not defined, current application is not an AppImage

@nezia1 nezia1 force-pushed the package-awakened-poe branch from e98e53f to 4c8f86f Compare September 11, 2024 22:14
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Sep 12, 2024
@nezia1 nezia1 requested a review from dotlambda September 12, 2024 15:53
@FliegendeWurst FliegendeWurst added the 12. first-time contribution This PR is the author's first one; please be gentle! label Nov 3, 2024
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 4, 2024
Comment on lines +21 to +22
name = "${pname}-${version}";
inherit src;
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails evaluation for me. I think, didn't test, you need to do today:

Suggested change
name = "${pname}-${version}";
inherit src;
inherit pname src version;

@FliegendeWurst FliegendeWurst marked this pull request as draft December 31, 2024 11:46
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` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: Awakened Poe Trade
5 participants