-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix(main/openjdk-{17,21}): adopt '.alternatives' system #25214
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
Conversation
d34a89b
to
25a40c4
Compare
) | ||
echo 'if [ "$#" = "3" ] && dpkg --compare-versions "$2" le "17.0.15-1"; then' > ./preinst | ||
echo ' echo "Removing older alternatives for openjdk-21 and openjdk-17"' >> ./preinst | ||
echo ' echo "This may take a while if mandoc package is installed, please wait..."' >> ./preinst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense for this please wait message to be at the end before the part where we actually remove the alternatives. I'll change this before I merge the PR. Apart from this, I'm open to other code reviews
I think we should definitely look into making the Thanks for taking the time to port this to the new system. |
By the way, I am currently writing a linter step for the |
25a40c4
to
6d4022d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at the first commit on this branch, it looks like you have a ghost version of bfa4e8c here.
6d4022d
to
9eb992b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess I should have waited 5 more seconds before that comment.
Yeah, fixed! Thanks. Messed up during rebase somehow. Fixed with multiple rebases locally :) |
@@ -0,0 +1,62 @@ | |||
Name: java | |||
Link: bin/java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This will most likely cause a regression on termux-pacman, reintroducing an issue from last year.
[Bug]: Java not work termux-pacman/termux-packages#86
There is an open PR by Robert to split update-alternatives
out into a separate subpackage to prevent this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it should not. termux_step_update_alternatives
is being called when generating pacman packages as well
termux_step_update_alternatives |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of the problem with pacman
and update-alternatives
is simply that currently, update-alternatives
might not be installed on pacman
-based Termux installations because it comes with dpkg
, I don't think there is any other inherent problem as far as I know, so I believe my PR should be acceptable to resolve any concerns regarding this.
The error shown in
could very reasonably have been produced by the absence of the update-alternatives
command while pacman
was running the openjdk-17
postinst script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Luckily, I decided to test my PR directly on my termux-pacman device myself,
as I expected, there weren't technically any problems with the update-alternatives
command itself, which seems to work properly on the termux-pacman device without the presence of dpkg
being installed at all,
however, that allowed me to notice that termux-pacman cannot yet use the postinst scripts of any of the packages that currently have .alternatives
files, because they don't have the block
if [ "$TERMUX_PACKAGE_FORMAT" = "pacman" ]; then
echo "post_install" > postupg
fi
anywhere in their build steps.
This led me to attempt to solve that problem a different way, which I have now added into the same PR, to make it a clustered "alternatives support for termux-pacman" PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the way I tried to do it would have created a folder named /data/data/com.termux/files/usr/var/lib/dpkg/alternatives
in termux-pacman, while that would have technically worked without errors, the presence of a folder named dpkg
in termux-pacman would have compromised the idea of alternative package manager in Termux, so it's necessary to create a different solution for termux-pacman.
I'm not sure whether this openjdk PR should be paused until after a different solution for termux-pacman is finished or not. Removing more pacman/postinst
files would create more errors in termux-pacman, but there are currently other errors in termux-pacman anyway, because of the previous removal of other pacman/postinst
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should move ahead with the merge considering that pacman is not officially supported
This fixes a lot of the mess that was created earlier. Now instead of creating new update-alternative entry for each manpage, binary and the etc/profile.d/java.sh file, there is a single alternative for java. This should make the switching of java versions much less painful. Just one update-alternatives command and everything is updated in one go. This also fixed the installation of openjdk-{17,21} taking an exorbitantly long amount of time as makewhatis is run evertime a manpage is changed. And since there were different update-alternative entries for each manpage, the manpage database was generated over 25 times! Making the install very slow. This changes to using slaves so the manpages are changed only once and the database is generated only once. 25x speed up. For users updating from older versions of openjdk, a preinst script is being provided that removes older update-alternatives entry for all the older stuff. A message is printed to stdout for users so that they know that the installation is going to take some time if mandoc is installed due to above mentioned problem.
libraries to linker Causes runtime linker errors without this. Regression caused in 38a7e95
9eb992b
to
45b319e
Compare
This fixes a lot of the mess that was created earlier. Now instead of creating new update-alternative entry for each manpage, binary and the etc/profile.d/java.sh file, there is a single alternative for java. This should make the switching of java versions much less painful. Just one update-alternatives command and everything is updated in one go.
This also fixed the installation of openjdk-{17,21} taking an exorbitantly long amount of time as makewhatis is run evertime a manpage is changed. And since there were different update-alternative entries for each manpage, the manpage database was generated over 25 times! Making the install very slow. This changes to using slaves so the manpages are changed only once and the database is generated only once. 25x speed up.
For users updating from older versions of openjdk, a preinst script is being provided that removes older update-alternatives entry for all the older stuff. A message is printed to stdout for users so that they know that the installation is going to take some time if mandoc is installed due to above mentioned problem.
Also earlier the alternatives were installed with different priorities j for the binaries as well as the manpages, which makes no sense. Now this has changed as well. Since every other file except for the java binary is installed as a slave of bin/java, every manpage and binary has the same priority. openjdk-21 is installed with higher priority (60) than openjdk-17 (40)