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

apkeditor: init at 1.4.1 #349470

Merged
merged 1 commit into from
Nov 1, 2024
Merged

apkeditor: init at 1.4.1 #349470

merged 1 commit into from
Nov 1, 2024

Conversation

UlyssesZh
Copy link
Member

@UlyssesZh UlyssesZh commented Oct 18, 2024

APKEditor: Powerful android apk resources editor.

This package installs a command APKEditor.

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

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Diff LGTM

pkgs/by-name/ap/apkeditor/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/package.nix Outdated Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 22, 2024
@UlyssesZh UlyssesZh force-pushed the apkeditor branch 2 times, most recently from 45b88f6 to be30d9a Compare October 23, 2024 16:30
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 23, 2024
pkgs/by-name/ap/apkeditor/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/package.nix Outdated Show resolved Hide resolved
@UlyssesZh UlyssesZh marked this pull request as draft October 24, 2024 17:21
@UlyssesZh UlyssesZh changed the title apkeditor: init at 1.4.1 apkeditor{,-bin}: init at 1.4.1 Oct 24, 2024
@UlyssesZh UlyssesZh marked this pull request as ready for review October 24, 2024 23:13
@UlyssesZh UlyssesZh requested a review from Atemu October 24, 2024 23:14
Copy link
Member

Choose a reason for hiding this comment

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

Given that we have a from-source version, do you think there's merit in the precompiled version?

Copy link
Member Author

Choose a reason for hiding this comment

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

The pre-compiled version is different from the from-source version in that it uses the binary libraries in the repo while the from-source version compiles those binary libraries from their original source. I cannot know what is the version of the libraries that the pre-compiled release use, so one of them may have bugs that the other one does not have.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah? Almost any software we are packaging in nixpkgs is not using the exact version of all dependencies as upstream does but usually that is not that big of a deal especially if it is just a patch or a minor release.

one of them may have bugs that the other one does not have.

Is that theoretical or did the from source compiled version actually not work as expected in your tests?

pkgs/by-name/ap/apkeditor/deps-apkeditor.json Outdated Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/package.nix Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/package.nix Outdated Show resolved Hide resolved
@UlyssesZh UlyssesZh force-pushed the apkeditor branch 2 times, most recently from c9a83c2 to 07a444a Compare October 25, 2024 16:14
@Atemu
Copy link
Member

Atemu commented Oct 26, 2024

This is starting to look pretty good.

One thing that sticks out a bit is that the file is quite long. Would it be possible to factor out the vendored deps into separate files perhaps?

@UlyssesZh UlyssesZh force-pushed the apkeditor branch 3 times, most recently from 785afb8 to 2908c8e Compare October 26, 2024 19:01
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Some final nits.

I have ran the binary and it successfully decompiles an APK.

pkgs/by-name/ap/apkeditor/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/package.nix Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/jcommand/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/smali/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/ap/apkeditor/smali/default.nix Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 26, 2024
@Atemu
Copy link
Member

Atemu commented Oct 27, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 349470


x86_64-linux

✅ 2 packages built:
  • apkeditor
  • apkeditor-bin

@Atemu
Copy link
Member

Atemu commented Oct 27, 2024

Could you split the package inits into separate commits? Then we can merge.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 27, 2024
@SuperSandro2000
Copy link
Member

Before we merge, I want to take another look at #349470 (comment) because I think if the source variant is buggy, we should not package it and if it isn't then we don't need the binary variant. If theoretical something could not work but in practice it does, I think we should drop the binary package.

@UlyssesZh
Copy link
Member Author

UlyssesZh commented Oct 28, 2024

It's theoretical. I see your point. Normally I won't package with the binary if I can build from source.

APKEditor manages its dependencies by having the dependencies compiled as jar files in the libs dir instead of letting Gradle fetch from Maven or something. This untypical way of managing dependencies made me re-thought about this. Normally, if building from source with different version of dependency doesn't work, we can know which difference caused this because we can see the codes. For APKEditor, it would be harder to troubleshoot in this case because we technically cannot see the source codes of its dependencies.

I do not know what people would do in this case by convention. I briefly tested the package built from source, and it worked, so if you think we can delete the package built from binary, I am OK with that.

@Atemu
Copy link
Member

Atemu commented Oct 30, 2024

Yeah let's skip the binary package for now.

@Atemu
Copy link
Member

Atemu commented Oct 31, 2024

gradle_6 was dropped yesterday: #352490. You'll have to make it work with gradle_7 or newer.

@UlyssesZh UlyssesZh changed the title apkeditor{,-bin}: init at 1.4.1 apkeditor: init at 1.4.1 Oct 31, 2024
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

You'd typically fetch the patches but this is fine too as they're simple enough.

@Atemu Atemu merged commit 1382d66 into NixOS:master Nov 1, 2024
29 of 30 checks passed
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.

4 participants