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

Dependency Check #68

Closed
wants to merge 3 commits into from
Closed

Dependency Check #68

wants to merge 3 commits into from

Conversation

databoose
Copy link

Add check for required dependecy for debian/arch based systems. Installs if non-existent.

Add check for required dependecy for debian/arch based systems. Installs if non-existent.
fixed debian and arch/arch-based detection, and installs their approriate required dependencies to build.
@thrimbor
Copy link
Member

There are countless different distributions and package managers, and it's impractical to support all (or even most).
I'd prefer to keep it simple instead, by better documenting the dependency in the README and maybe a simple check for xsltproc at the top of the build script.
That way we don't have to deal with any distribution-specific behavior or keep track of renamed packages etc.

…ng" derivative.

Any arch-based system that: 

1. implements the official arch repository
2. implements the base toolkit of arch (for example, pacman)

Will be detected by this script via utilization of ID_LIKE

see : https://www.freedesktop.org/software/systemd/man/latest/os-release.html for more documentation on ID_LIKE
@databoose
Copy link
Author

databoose commented Sep 14, 2024

There are countless different distributions and package managers, and it's impractical to support all (or even most). I'd prefer to keep it simple instead, by better documenting the dependency in the README and maybe a simple check for xsltproc at the top of the build script. That way we don't have to deal with any distribution-specific behavior or keep track of renamed packages etc.

I have changed the script to include any arch, or arch-based system (not counting for arch deviation distros like manjaro that do not classify themselves as arch in ID_LIKE).

For clarification on what ID_LIKE is, and what it entails, the documentation can be viewed here : https://www.freedesktop.org/software/systemd/man/latest/os-release.html

Although i do agree the documentation should clarify this package is needed, this requires knowing the package name to even install, which is just about the same difficulty as what'd being done here. (the debian and arch package names are totally different, for example).

"There are countless different distributions and package managers, and it's impractical to support all (or even most)."

This is a technically true statement but an extremely misleading one, the grand majority of systems, in this context of desktop users within linux, are based on debian , or if not, than arch, and the grand majority of desktop linux systems, use their respective default package manager, with debian being APT, and arch's being pacman.

For example, linux mint, ubuntu, zorin OS, all of these "based systems" still fully utilize both the full and official package repos for the "root" distros, they just add onto that base, they do not remove from the root of the distro, there is no confliction here except in very obscure distros. (in which this script wouldn't even pick up)

Just about all of these debian based systems are going to be using APT, and virtually all of these arch/arch based systems are going to be pacman

With arch, there is actually far less arch-based systems, the most notable coming to mind being endeavourOS, which again fully implements the official arch repo and adding non-conflicting features on top of that base.

What this would not cover, at least for now , is fedora and gentoo systems, however it is very safe to say that most linux users here would be covered on this script even now at this stage.

@databoose
Copy link
Author

databoose commented Sep 14, 2024

Additionally i'd like to add, that not adding these checks at all is literally just only a negative result and zero positive result, there is no risk or no downside to adding checks for popular linux desktop systems to have this package.

I had to spend an entire day figuring out what the problem was until i had realized i was simply missing this package, i think this would be a great addition that would prevent future users from encountering the same issue i had to.

This currently is only a mitigation, not a solution, but i think it is a very large and very significant mitigation and that certaintly it is better than nothing, and having people wondering why this project doesn't work. having to spend hours troubleshooting the issue is no good, i think we need a mitigation.

I am open to changing the idea perhaps but i can't really think of one that wouldn't require at least checking the general distro to know what the package is that needs to be installed.

@mborgerson
Copy link
Member

I appreciate the thought for convenience, but having a build script invoke the user's package manager to install a dependency seems surprising and unnecessary.

maybe a simple check for xsltproc at the top of the build script

I agree. Let's simply add a check for the tool.

@databoose
Copy link
Author

databoose commented Sep 20, 2024

I appreciate the thought for convenience, but having a build script invoke the user's package manager to install a dependency seems surprising and unnecessary.

maybe a simple check for xsltproc at the top of the build script

I agree. Let's simply add a check for the tool.

Fair enough although the dep would need to be checked for on a per-distro basis. which is essentially what we're already doing here with trying to automatically install the respective distro's package for the dependency, if they already have it, no harm done, the package is already detected as installed by the package manager.

But if most people agree that simply detection of the absecene of this library is a better solution, feel free to edit this code as, quite frankly i am terrible with bash (and i presume obviously this script would want to be kept bash)

Alternatively the readme could be edited to alert users that this dependency is required but i feel like alerting upon trying to build would be more foolproof, in my opinion, if all we want to do is simply alert but not try to install the package.

@databoose
Copy link
Author

databoose commented Sep 21, 2024

I will be closing this solution as the concensus seems to be that this is not the right approach, i will be closing the PR, but i'm open to new ideas regarding this.

@databoose databoose closed this Sep 21, 2024
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.

3 participants