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

arc_unpacker: init at b9843a1 #97028

Merged
merged 1 commit into from Nov 14, 2020
Merged

Conversation

midchildan
Copy link
Member

Motivation for this change

arc_unpacker is a tool to extract files from visual novel archives.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@midchildan
Copy link
Member Author

/marvin opt-in
/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added the marvin label Sep 3, 2020
@marvin-mk2
Copy link

marvin-mk2 bot commented Sep 3, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

pkgs/tools/archivers/arc_unpacker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/archivers/arc_unpacker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/archivers/arc_unpacker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/archivers/arc_unpacker/install.cmake Outdated Show resolved Hide resolved
@IvarWithoutBones
Copy link
Member

Commits should be squashed together, and the commit message should be using the new version, instead of the rev. Other than that, LGTM :)

@midchildan
Copy link
Member Author

squashed ;-)

Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 97028 1

1 package built:
- arc_unpacker

Copy link
Member

@vikanezrimaya vikanezrimaya left a comment

Choose a reason for hiding this comment

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

/status needs_merger

pkgs/tools/archivers/arc_unpacker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/archivers/arc_unpacker/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Thanks @IvarWithoutBones and @kisik21 for the reviews!

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Noticed ofBorg failures last-minute. Looks like you moved the -unstable part into pname, which then made the repo wrong. So nix cannot find the source tarball anymore. (@kisik21 you see? :P)

src = fetchFromGitHub {
owner = "vn-tools";
repo = pname;
# the latest release (0.11) doesn't build
Copy link
Member

Choose a reason for hiding this comment

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

Since we need another iteration anyway, please add a link to the upstream issue in the comment.

@midchildan
Copy link
Member Author

Oops, fixed.

@vikanezrimaya
Copy link
Member

Well, now I see 😜

@timokau timokau merged commit a638df7 into NixOS:master Nov 14, 2020
@midchildan midchildan deleted the package/arc_unpacker branch November 14, 2020 14:28
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.

None yet

4 participants