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

termux-tools: improving pkg to work with pacman #10486

Closed
wants to merge 3 commits into from

Conversation

Maxython
Copy link
Member

@Maxython Maxython commented May 4, 2022

Close #10460

Copy link
Member

@thunder-coding thunder-coding left a comment

Choose a reason for hiding this comment

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

Hey there, thanks for the PR. Some suggestions from my side, everything else seems good.
Also thanks for showing that using an alternative package manager with Termux (and not just apt) is completely possible!

packages/termux-tools/pkg Outdated Show resolved Hide resolved
packages/termux-tools/pkg Outdated Show resolved Hide resolved
packages/termux-tools/pkg Show resolved Hide resolved
packages/termux-tools/pkg Outdated Show resolved Hide resolved
packages/termux-tools/pkg Outdated Show resolved Hide resolved
@Sarisan

This comment was marked as off-topic.

@agnostic-apollo
Copy link
Member

  1. Why are you using TERMUX_PACKAGE_FORMAT conditionals? The variable is only exported by build-package.sh and not in terminal sessions in the app.

debian|pacman) TERMUX_PACKAGE_FORMAT="$1";;

  1. The TERMUX_MAIN_PACKAGE_FORMAT is only exported by login script so will only be available for login shells and not for non-login shells or termux plugin app sessions and tasks.

export TERMUX_MAIN_PACKAGE_FORMAT="@TERMUX_PACKAGE_FORMAT@"

  1. Once Enable support for android 5/6 for termux-app termux-app#2740 is merged (possibly today/tomorrow), then TERMUX_PACKAGE_MANAGER will be exported with the values apt and pacman for all sessions/tasks. The app has no knowledge of TERMUX_MAIN_PACKAGE_FORMAT or TERMUX_PACKAGE_FORMAT and hence should not be used anymore. The TERMUX_MAIN_PACKAGE_FORMAT should either be removed or deprecated from next app version that supports pacman bootstraps, since different package managers will have different APKs and users shouldn't switch package manager manually in $PREFIX.

  2. The TERMUX_MAIN_PACKAGE_FORMAT=debian should also be replaced for on device builds in build scripts with TERMUX_PACKAGE_MANAGER=apt for better consistency and singular variable.

    The TERMUX_PACKAGE_FORMAT should be replaced with something like TERMUX_BUILD_PACKAGE_FORMAT.

  3. At the start of the pkg and termux-info scripts there should be a check of if TERMUX_PACKAGE_MANAGER is actually set to valid package managers apt or pacman, with the default being apt if variable is unset, instead of blindly executing code since no else conditions exist currently for invalid/unset values.

@Maxython
Copy link
Member Author

Maxython commented May 6, 2022

Why are you using TERMUX_PACKAGE_FORMAT conditionals? The variable is only exported by build-package.sh and not in terminal sessions in the app.

Thanks for noticing.

Once Enable support for android 5/6 for termux-app termux-app#2740 is merged (possibly today/tomorrow), then TERMUX_PACKAGE_MANAGER will be exported with the values apt and pacman for all sessions/tasks. The app has no knowledge of TERMUX_MAIN_PACKAGE_FORMAT or TERMUX_PACKAGE_FORMAT and hence should not be used anymore. The TERMUX_MAIN_PACKAGE_FORMAT should either be removed or deprecated from next app version that supports pacman bootstraps, since different package managers will have different APKs and users shouldn't switch package manager manually in $PREFIX.

That is, now you need to configure the package compiler, termux packages for android 7+ and termux packages for android 5/6? If so, then doing it right now is quite problematic.

@agnostic-apollo
Copy link
Member

Welcome.

You don't need to add support for android 5/6. You/We only need to add support for pacman-android-7 variant, which should already be possible with the generate-bootstrap.sh update in #10276. The workflow and app will require some changes, which I'll do.

@Maxython
Copy link
Member Author

Maxython commented May 6, 2022

@thunder-coding, I tried to keep the code as short as possible. That unclosed conversation was not carried out because I had no idea how to do it.

@thunder-coding
Copy link
Member

@thunder-coding, I tried to keep the code as short as possible. That unclosed conversation was not carried out because I had no idea how to do it.

Will look into that part, thanks for resolving the other changes! And yeah, I ought to be more brief when doing code reviews. Sorry for the inconvenience caused due to it.

@Maxython
Copy link
Member Author

@thunder-coding

Copy link
Member

@thunder-coding thunder-coding left a comment

Choose a reason for hiding this comment

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

Seems good! Just a single change requested this time. :) Also make sure that you don't use merge commits as they cause problems when building from master

packages/termux-tools/pkg Outdated Show resolved Hide resolved
@Maxython
Copy link
Member Author

I can’t merge commits, other commits are set when merging

@thunder-coding
Copy link
Member

Thanks merged manually in f1a83c5

agnostic-apollo pushed a commit to termux/termux-tools that referenced this pull request Aug 1, 2022
Co-authored-by: Maxython <mixython@gmail.com>
Closes: termux/termux-packages#10486
agnostic-apollo pushed a commit to termux/termux-tools that referenced this pull request Aug 1, 2022
Co-authored-by: Maxython <mixython@gmail.com>
Closes: termux/termux-packages#10486
Grimler91 pushed a commit to termux/termux-tools that referenced this pull request Aug 2, 2022
Co-authored-by: Maxython <mixython@gmail.com>
Closes: termux/termux-packages#10486
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: termux-tools pkg command
4 participants