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

fix broken if condition with modified xorg.conf #57

Merged
merged 3 commits into from
Jan 24, 2021

Conversation

fennifith
Copy link
Contributor

Fixes issue #56 - found a way to use grep to check for "if the output contains this specific word", which I think was the original intent of that functionality. Chaining cat | grep | cut | grep does seem a bit complex, I'm not sure if there's a simpler way to achieve this - but it should fix the problem, anyway.

@hertg
Copy link
Owner

hertg commented Jan 21, 2021

Hey, thanks for the PR!
I can't look at it right now, I might be able to do that on the weekend. 😄

@ewagner12
Just wanted to notify you as it's a change to the code you submitted. Let me know if you have any feedback

@ewagner12
Copy link
Contributor

Hi, I'm ok with this PR as it seems to work ok with my system, but speaking of cleaner solutions, how about this:
if [ ${mode} = "egpu" ] && ! cat $xfile_egpu | grep -Eiq 'Driver[[:space:]]*\"nvidia'; then
let me know if replacing the problem line with that would fix your issue as well.

You're correct that it's supposed to look for the single word "nvidia" in the xorg.conf.egpu file and only go into the if statement only if it doesn't find it. Specifically, this was designed so that AMD cards or nvidia cards using the open source "nouveau" driver would go into the if statement, but nvidia cards using the proprietary "nvidia" driver won't.

@hertg
Copy link
Owner

hertg commented Jan 23, 2021

Thanks for you feedback.

You're correct that it's supposed to look for the single word "nvidia" in the xorg.conf.egpu file and only go into the if statement only if it doesn't find it.

Depending on that statement, i believe we could simplify the regex so it matches whenever the word nvidia is found on the Driver line, right? So my proposal would be to change the regex itself to simply Driver.*nvidia. @ewagner12 Do you see any problems with that?

Also, I just realized that piping the cat output into grep seems unnecessary, we could change that to pass the file directly as a grep argument: grep -Eiq 'Driver.*nvidia' $xfile_egpu.

The full line being:

# when mode is 'egpu' and no 'nvidia' driver is used
if [ ${mode} = "egpu" ] && ! grep -Eiq 'Driver.*nvidia' $xfile_egpu; then

Please report if I missed anything.

@fennifith Would be great if you could give your feedback and test my proposal on your system. If that works well for you, you could push a new commit with that change and I'll then accept the PR.

@fennifith
Copy link
Contributor Author

Yep, this works for me! Definitely an improvement over my original fix :)
I will commit that momentarily.

I believe this does mean that any driver containing "nvidia" will also be matched (which was not the case before), such as other-nvidia or, idk, nounvidiau if someone horribly misspells nouveau - but I don't know of any other drivers named as such (and if there are, this behavior might be desirable anyway).

@hertg hertg merged commit 49d07ad into hertg:master Jan 24, 2021
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