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

Mybash dependencies #751

Closed

Conversation

cartercanedy
Copy link
Contributor

@cartercanedy cartercanedy commented Oct 3, 2024

Type of Change

  • Bug fix

Description

Corrects the dependency executable tests

Testing

tested on a fresh install of openSUSE Leap 15.6 vm

Impact

Should make mybash run as intended on fresh installs

Issues / other PRs related

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

Copy link
Contributor

@nnyyxxxx nnyyxxxx left a comment

Choose a reason for hiding this comment

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

revert these changes, command_exists only checks if the first package exists as a command. Also there is no issue with how it is, even if the command does not exist it will still run due to the "!", this PR is pretty redundant.

@cartercanedy
Copy link
Contributor Author

cartercanedy commented Oct 4, 2024

Have you tried running the command on a fresh headless installation? It fails because there are missing dependencies, particularly fc-cache that comes with fontconfig. Please try the current deployment on a fresh headless VM and tell me what happens

@cartercanedy
Copy link
Contributor Author

for context, here's the issue manifested.
the command fails since it's current implementation inadequately verifies that the dependencies are installed
image

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 4, 2024

for context, here's the issue manifested. the command fails since it's current implementation inadequately verifies that the dependencies are installed image

again, re read what i said, the simple solution to this is to use 1 pkg for detection. All you're doing here is bloating up the deps function.

@cartercanedy
Copy link
Contributor Author

Your suggestion as implemented would not have resolved the issue

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

Your suggestion as implemented would not have resolved the issue

explain how it wouldnt resolve the issue?

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

there literally is no issue here, just remove every other dep from the command exists line lmao

@cartercanedy
Copy link
Contributor Author

You've refuse to recreate the environment that I've already been very clear about, and are still choosing to insist on your incorrect understanding of the issue.

It is an issue, this fixes it without additional regressions

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

You've refuse to recreate the environment that I've already been very clear about, and are still choosing to insist on your incorrect understanding of the issue.

It is an issue, this fixes it without additional regressions

again there is no issue here, what you're doing is bloating up the deps function..

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

there is no need to verify if deps are installed if they are literally getting installed beforehand

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

anyways, i cant even reproduce the issue you're having. The script works perfectly fine for me

@cartercanedy
Copy link
Contributor Author

anyways, i cant even reproduce the issue you're having. The script works perfectly fine for me

You're being ignorant at this point

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

anyways, i cant even reproduce the issue you're having. The script works perfectly fine for me

You're being ignorant at this point

again there is no issue here, as i said above i cant reproduce. The fontconfig package is literally getting installed, WHY check for these deps after they are installed?? you are just bloating up the func

@cartercanedy
Copy link
Contributor Author

It's incredible how hostile you're being right now

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

It's incredible how hostile you're being right now

no hostility is being expressed, i am asking a question as seen here

The fontconfig package is literally getting installed, WHY check for these deps after they are installed?? you are just bloating up the func

the capped "why" is to show frustration over you not getting the point i'm trying to make

@cartercanedy
Copy link
Contributor Author

cartercanedy commented Oct 5, 2024

The point of checking for dependencies is to preserve efficiency. An alternative is to just blindly install the dependencies without worrying about checking for deps. The script fails as it's currently written; there's multiple solutions, and I can only implement one. You've repeatedly insisted that there is no issue. If that's your point, then you are either mistaken or intentionally being combative.

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

The point of checking for dependencies is to preserve efficiency. An alternative is to just blindly install the dependencies without writing about checking for deps. The script fails as it's currently written; there's multiple solutions, and I can only implement one. You've repeatedly insisted that there is no issue. If that's your point, then you are either mistaken or intentionally being combative.

if you are trying to implement dep checking, then why arent you doing this for every script? also as ive said multiple times, if you are installing a set of packages why would they fail? Have you ever been in the process of installing a package and it fails on you? I dont think so... Going by that, there is no issue here, if you're installing fontconfig then fc-cache should work perfectly fine. no need to check for deps all that does is create unnecessary overhead and more time spent on the users end to finish the script

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

also to address the statement above about the command_exists bash, the script is literally installing BASH and is implementing bash configurations, also bash isnt the only shell, and its certainly not one that the user has to keep on their system after installation; alternatives exist: such as Zsh, nushell, fish, and multiple others.

@cartercanedy
Copy link
Contributor Author

Then make the pr yourself. @ChrisTitusTech can decide whether or not to merge. I'm not going to continue this conversation

@cartercanedy
Copy link
Contributor Author

cartercanedy commented Oct 5, 2024

As a final note, it's very disheartening to see how you've reacted to this bug report. I've made these changes in good faith, but your deliberately rude comments are not collaborative in the slightest. Your first words in response to my bug report were "you're wrong", and I believe that makes my point very clear.

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

As a final note, it's very disheartening to see how you've reacted to this bug report. I've made these changes in good faith, but your deliberately rude are not collaborative I'm the slightest. Your first words in response to my bug report were "you're wrong", and I believe make my point very clear.

i've explained to you many times that i cannot reproduce the issue you're having, i have even gone to the lengths of firing up a VM, and could not reproduce in there either.

and as said above

if you are trying to implement dep checking, then why arent you doing this for every script? also as ive said multiple times, if you are installing a set of packages why would they fail? Have you ever been in the process of installing a package and it fails on you? I dont think so... Going by that, there is no issue here, if you're installing fontconfig then fc-cache should work perfectly fine. no need to check for deps all that does is create unnecessary overhead and more time spent on the users end to finish the script

Please answer this i would like to hear your thoughts, it explains what i'm trying to convey very well

@cartercanedy
Copy link
Contributor Author

I'm not going to walk through every line of code with you. Just evaluating the code statically, it was clear to me what the issue was. I implemented the fix, the issue was resolved in my edge-case environment, and trying the script still in my dev environment still worked, indicating that the program was not correct before, and is correct now. The dependencies were not being installed if not present prior to these changes, and now they are. It is an issue, it is not a "non-existent" issue, as you stated. Read my reproduction steps, it's dead simple to reproduce.

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

I'm not going to walk through every line of code with you. Just evaluating the code statically, it was clear to me what the issue was. I implemented the fix, the issue was resolved in my edge-case environment, and trying the script still in my dev environment still worked, indicating that the program was not correct before, and is correct now. The dependencies were not being installed if not present prior to these changes, and now they are. It is an issue, it is not a "non-existent" issue, as you stated. Read my reproduction steps, it's dead simple to reproduce.

theres no issue here

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

the script works perfectly fine for me in and out of VM. Can't reproduce this issue at all

Copy link
Contributor

@lj3954 lj3954 left a comment

Choose a reason for hiding this comment

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

image

Yes, there is an issue with the previous syntax. And yes, that would cause a problem if the first command (bash) exists (which it nearly certainly will) while any of the others don't.

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

both of these versions are basically doing the same thing

  1. previous version installs the deps (and does it correctly as this is in every other script
  2. new version installs the deps then checks if those deps are installed

This new version bloats the script up, not sure what your issue is here @cartercanedy and i dont know what you're trying to accomplish by adding this complexity onto the script; as i've said many times the issue you're facing is unreproducable meaning i can NOT reproduce this, in and out of a VM, tested on host (Arch Linux) and in a VM (openSUSE).

This reference
image
is wrong because of the !, infact it should never fail because the ! makes it so if any of them are missing it will proceed and install and since bash-completion and fontconfig aren't executables it will always run through. Along with this i suggested removing all of the deps from the command_exists function and replacing them with just BASH but of course you declined and that is the: quote me on this "solution to your nonexistent problem" as stated in the images above.

These changes do the following:

  1. cause unnecessary overhead
  2. accomplish the same thing as the previous version except for dep checking (which is not necessary)
  3. potential for more unnecessary maintenance burden

please just close this non-sense, i have gave you a solution and you have declined it (for reasons unknown) this is just disgusting.

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

image

Yes, there is an issue with the previous syntax. And yes, that would cause a problem if the first command (bash) exists (which it nearly certainly will) while any of the others don't.

if that is true then why do all of the other scripts work? many people have ran through them including myself and have encountered 0 issues. Please explain

@lj3954
Copy link
Contributor

lj3954 commented Oct 5, 2024

@nnyyxxxx There is a syntax error in the current code. We're passing multiple arguments to a function which only can accept one. If you'd like to propose instead allowing the command_exists function to check through a list of arguments rather than just one (using a loop, much like this), you may. But this is code with a syntax error, and this PR just fixes said error. We don't need this pointless tirade about "performance" and how we "don't need these changes" again, it's ridiculous.

The script works for you because all of these commands exist on your system. If one didn't exist on someone else's system, we'd have an issue. command_exists does not take in more than one parameter, the function is not being called correctly and all of the commands past the first are being completely disregarded.

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

@nnyyxxxx There is a syntax error in the current code. We're passing multiple arguments to a function which only can accept one. If you'd like to propose instead allowing the command_exists function to check through a list of arguments rather than just one (using a loop, much like this), you may. But this is code with a syntax error, and this PR just fixes said error. We don't need this pointless tirade about "performance" and how we "don't need these changes" again, it's ridiculous.

The script works for you because all of these commands exist on your system. If one didn't exist on someone else's system, we'd have an issue. command_exists does not take in more than one parameter, the function is not being called correctly and all of the commands past the first are being completely disregarded.

as i have said in my previous posts in this thread i know that only 1 argument can be passed to command_exists and i have suggested the change to simplify it down to just bash many times, what hes doing here is completely unnecessary

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

@lj3954
Copy link
Contributor

lj3954 commented Oct 5, 2024

The other programs are also dependencies, correct? "Simplifying" the dependency check to not check for dependencies isn't a solution to anything. We can't expect 'unzip' or 'fontconfig' to be installed just because bash is, those aren't dependencies of bash.

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

image

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

The other programs are also dependencies, correct? "Simplifying" the dependency check to not check for dependencies isn't a solution to anything. We can't expect 'unzip' or 'fontconfig' to be installed just because bash is, those aren't dependencies of bash.

except it is a solution when literally every other script does it, if we want a solution to the 1 dep checking issue, then we can alter the command_exists function in common-script instead of modifying every script to include this dep checking nonsense

@lj3954
Copy link
Contributor

lj3954 commented Oct 5, 2024

If every script is calling a function with the wrong amount of parameters, then they're just all wrong. Indeed, you can suggest instead modifying the command_exists function to allow passing in multiple parameters (with a loop, much like the code proposed in this PR. You can't pass multiple arguments directly into command -v). But suggesting that you only check for one of the numerous dependencies, or claiming that there's "no issue" because you can't reproduce it yourself, isn't helpful.

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

image

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 5, 2024

#762

is an easier way to solve this "issue" without having to add dep checks to every script

@cartercanedy
Copy link
Contributor Author

both of these versions are basically doing the same thing

  1. previous version installs the deps (and does it correctly as this is in every other script
  2. new version installs the deps then checks if those deps are installed

This new version bloats the script up, not sure what your issue is here @cartercanedy and i dont know what you're trying to accomplish by adding this complexity onto the script; as i've said many times the issue you're facing is unreproducable meaning i can NOT reproduce this, in and out of a VM, tested on host (Arch Linux) and in a VM (openSUSE).

This reference
image
is wrong because of the !, infact it should never fail because the ! makes it so if any of them are missing it will proceed and install and since bash-completion and fontconfig aren't executables it will always run through. Along with this i suggested removing all of the deps from the command_exists function and replacing them with just BASH but of course you declined and that is the: quote me on this "solution to your nonexistent problem" as stated in the images above.

These changes do the following:

  1. cause unnecessary overhead
  2. accomplish the same thing as the previous version except for dep checking (which is not necessary)
  3. potential for more unnecessary maintenance burden

please just close this non-sense, i have gave you a solution and you have declined it (for reasons unknown) this is just disgusting.

This is probably the best example of a code of conduct violation on your part. If it's not, I don't know what would qualify. You've been hostile to two contributors, myself and @lj3954, which is the only actually disgusting behavior on display. @ChrisTitusTech I'd like to hear your opinion on this,

@cartercanedy
Copy link
Contributor Author

cartercanedy commented Oct 5, 2024

This is also completely omitting any other context. Looking strictly at the course of this conversation, your behavior is completely out of line and totally antithetical to the open source process. Looking at other conversations, though, it's clear that this is a chronic issue and needs to be addressed

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 6, 2024

just close this, i have provided a better solution to this problem in #762

@cartercanedy
Copy link
Contributor Author

cartercanedy commented Oct 6, 2024

Closing in favor of #762

@lj3954 lj3954 mentioned this pull request Nov 7, 2024
1 task
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.

Mybash deps check is bugged
3 participants