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

Update Debian and Ubuntu Deps #90

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Conversation

tbleher
Copy link
Contributor

@tbleher tbleher commented Apr 13, 2024

This fixes two minor issues I found while trying to install on Ubuntu 22.04. Note that ensure-python.sh doesn't work on Ubuntu 22.04 yet, since winehq-staging is not found in the repo added to apt. But I think these two bugfixes are still worth merging.

@thw26
Copy link
Collaborator

thw26 commented Apr 15, 2024

Thank you for the contribution!

Do you know if these package changes apply to all Debian variants?

If so, could you also modify the following lines?

if shutil.which('apt') is not None: # debian, ubuntu
config.PACKAGE_MANAGER_COMMAND_INSTALL = "apt install -y"
config.PACKAGE_MANAGER_COMMAND_REMOVE = "apt remove -y"
config.PACKAGE_MANAGER_COMMAND_QUERY = "dpkg -l | grep -E '^.i '" # IDEA: Switch to Python APT library? See https://github.com/FaithLife-Community/LogosLinuxInstaller/pull/33#discussion_r1443623996
config.PACKAGES = "binutils cabextract fuse wget winbind"
config.L9PACKAGES = "" # FIXME: Missing Logos 9 Packages
config.BADPACKAGES = "appimagelauncher"

If this is Ubuntu specific, we will need to add a flag for distro. Compare the following:

https://github.com/FaithLife-Community/LogosLinuxInstaller/pull/82/files

Copy link
Collaborator

@thw26 thw26 left a comment

Choose a reason for hiding this comment

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

Initial review. Needs some minor changes.

scripts/ensure-python.sh Outdated Show resolved Hide resolved
@n8marti
Copy link
Collaborator

n8marti commented Apr 15, 2024

This fixes two minor issues I found while trying to install on Ubuntu 22.04. Note that ensure-python.sh doesn't work on Ubuntu 22.04 yet, since winehq-staging is not found in the repo added to apt. But I think these two bugfixes are still worth merging.

ensure-python.sh shouldn't be affected by whether or not winehq-staging package is installed. But if you are talking about the README.md instructions, they have you first install winehq's sources file, which is what makes winehq-staging available to apt. However, I see that those instructions are probably incomplete in that there should be a sudo apt update command in between the sudo wget command and the sudo apt install command. sudo apt update makes apt look at the new winehq apt source for the first time after adding the the sources file in the earlier step.

@n8marti
Copy link
Collaborator

n8marti commented Apr 16, 2024

I see that those instructions are probably incomplete in that there should be a sudo apt update command in between the sudo wget command and the sudo apt install command.

This particular oversight has been fixed in 6f8d420

@tbleher
Copy link
Contributor Author

tbleher commented Apr 20, 2024

Thank you for the contribution!

Do you know if these package changes apply to all Debian variants?

Yes - should have mentioned this in the commit message. The package names are the same in Debian and Ubuntu.

If so, could you also modify the following lines?

if shutil.which('apt') is not None: # debian, ubuntu
config.PACKAGE_MANAGER_COMMAND_INSTALL = "apt install -y"
config.PACKAGE_MANAGER_COMMAND_REMOVE = "apt remove -y"
config.PACKAGE_MANAGER_COMMAND_QUERY = "dpkg -l | grep -E '^.i '" # IDEA: Switch to Python APT library? See https://github.com/FaithLife-Community/LogosLinuxInstaller/pull/33#discussion_r1443623996
config.PACKAGES = "binutils cabextract fuse wget winbind"
config.L9PACKAGES = "" # FIXME: Missing Logos 9 Packages
config.BADPACKAGES = "appimagelauncher"

I'm a bit confused about the difference between these lists. One list for Debian/Ubuntu currently reads:

mktemp patch lsof wget find sed grep gawk tr winbind cabextract x11-apps bc

The other reads:

binutils cabextract fuse wget winbind

I did not find any usage of patch in the installer, and sed, bc and lsof also seems unused. For binutils I'm not completely sure (since it contains a number of binaries), but I don't see any direct usage.
OK, looking a bit further, sed and binutils seems to be used by winetricks.

I'll upload a patch to unify the two lists, but I guess patch, bc and lsof could be removed? Are these remnants of the old installer, or are they still needed?

- mktemp and tr are part of package coreutils
- find is part of package findutils

Note: both findutils and coreutils are marked as essential, so should
always be installed. So listing these packages is not strictly necessary,
but it also doesn't really hurt.

Sync the package lists between the README.md and utils.py. Switch to fuse3
in utils.py, to match the description in the README.
@tbleher
Copy link
Contributor Author

tbleher commented Apr 20, 2024

ensure-python.sh shouldn't be affected by whether or not winehq-staging package is installed. But if you are talking about the README.md instructions, they have you first install winehq's sources file, which is what makes winehq-staging available to apt. However, I see that those instructions are probably incomplete in that there should be a sudo apt update command in between the sudo wget command and the sudo apt install command. sudo apt update makes apt look at the new winehq apt source for the first time after adding the the sources file in the earlier step.

Sorry, I guess I mixed things up during testing. Thank you for the correction and the quick fix!

@n8marti
Copy link
Collaborator

n8marti commented Apr 20, 2024

I did not find any usage of patch in the installer, and sed, bc and lsof also seems unused. For binutils I'm not completely sure (since it contains a number of binaries), but I don't see any direct usage. OK, looking a bit further, sed and binutils seems to be used by winetricks.

Regarding winetricks' dependencies: This seems like a bit of a can of worms. Here's what I see in the winetricks script:

$ cat /usr/bin/winetricks | grep -EnB1 '#\s+sudo .* install'
42-# On Ubuntu (20.04 and newer), the following line can be used to install all the prerequisites:
43:#    sudo apt install aria2 binutils cabextract fuseiso p7zip-full pkexec tor unrar unzip wine xdg-utils xz-utils zenity
--
45-# On older Ubuntu, the following line can be used to install all the prerequisites:
46:#    sudo apt install aria2 binutils cabextract fuseiso p7zip-full policykit-1 tor unrar unzip wine xdg-utils xz-utils zenity
--
48-# On Fedora, these commands can be used (RPM Fusion is used to install unrar):
49:#    sudo dnf install https://download1.rpmfusion.org/free/fedora/rpmfusion-free-release-$(rpm -E %fedora).noarch.rpm https://download1.rpmfusion.org/nonfree/fedora/rpmfusion-nonfree-release-$(rpm -E %fedora).noarch.rpm
50:#    sudo dnf install binutils cabextract fuseiso p7zip-plugins polkit tor unrar unzip wget wine xdg-utils xz zenity

@thw26 What do you think about these? Should we copy them all into our own prereq lists? (Not exactly realistic, since even wine is included in that list.) Should we dig a bit deeper and figure out exactly which ones are needed for Logos and leave the rest out? (Maybe this was already done previously before I came along.)

@n8marti
Copy link
Collaborator

n8marti commented Apr 20, 2024

I'll upload a patch to unify the two lists, but I guess patch, bc and lsof could be removed? Are these remnants of the old installer, or are they still needed?

I would guess that they're remnants from when LogosLinuxInstaller was written in bash and can be removed, but I'll let @thw26 confirm.

@thw26
Copy link
Collaborator

thw26 commented Apr 20, 2024

Several of these, yes, are remnants of the old installer, like bc and lsof. I think patch was used in the old installer for Logos 9.

I reviewed the bash script and found no use of patch there. Perhaps it was part of the original developer's work but is no longer needed. I'm not sure what patch would have been used for.

lsof was used for wait_process_using_dir()

bc was used for version checking.

In #95, I note that both wget and curl are deps—we can probably drop curl as our stuff is using python, right? Do we even need wget then?

We are using grep in the Python package manager command query function.

sed and awk would have been used for a variety of things. awk is still used in the Python CODENAME var. sed is still used in the Python utils.steam_postinstall_dependencies().


The winetricks dependencies add a different concern though—the only way to confirm that is to test.
If we can run a single install on a clean system, we should be able to verify if we bump into issues with winetricks for our purposes.

@thw26
Copy link
Collaborator

thw26 commented Apr 20, 2024

For Ubuntu, should we add a command to add the PPA for Ubuntu users or leave as is? In time, with enough work from upstream, we won't need the dev or staging branch.

EDIT: cf. #102.

@thw26 thw26 changed the title Minor fixes Update Debian and Ubuntu Deps Apr 20, 2024
@n8marti
Copy link
Collaborator

n8marti commented Apr 20, 2024

In #95, I note that both wget and curl are deps—we can probably drop curl as our stuff is using python, right? Do we even need wget then?

We're not using wget, but winetricks uses it (or alternatively aria2, if it's installed, I think).

@thw26
Copy link
Collaborator

thw26 commented Jun 1, 2024

cabextract is used by winetricks

@thw26
Copy link
Collaborator

thw26 commented Jun 14, 2024

Merging this. We can deal with winetricks deps in another issue.

@thw26 thw26 merged commit 312599c into FaithLife-Community:main Jun 14, 2024
@n8marti
Copy link
Collaborator

n8marti commented Jun 15, 2024

Unfortunately, I just realized that the Ubuntu package dependency fuse was changed to fuse3. This was probably necessary for newer Ubuntu, but it breaks the dependency check for 20.04, which is still supported until 2025. I've opened a new issue.

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.

None yet

3 participants