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 for PopOS os_release problem #311

Merged
merged 8 commits into from
Dec 19, 2021

Conversation

emilyastranova
Copy link
Contributor

Fixes issue #276

@bobslept
Copy link
Contributor

Nice! Great improvement for the PopOS users. Maybe before offering this option to the user, check if /etc/pop-os/os-release exists so that you are for sure being on the PopOS distro which causes this error?

@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Dec 17, 2021

Nice! Great improvement for the PopOS users. Maybe before offering this option to the user, check if /etc/pop-os/os-release exists so that you are for sure being on the PopOS distro which causes this error?

Absolutely agree, let's run this block only if detected distro is pop os.

@emilyastranova
Copy link
Contributor Author

How does this look?

@bobslept
Copy link
Contributor

bobslept commented Dec 18, 2021

Almost there, I would just do it like this:

try:
    dist_name = distro.id()
except PermissionError:
    # Current work-around for distros like Pop!_OS where symlink causes permission issues
    print("Warning: Cannot get distro name")
    if os.path.exists("/etc/pop-os/os-release"):
        print("Pop!_OS detected")
        print("Pop!_OS uses a symbolic link for the os-release file, this causes issues \
            and can be fixed by converting to a hard link")
        print("Attempting to change symlink to hard link for /etc/os-release -> /etc/pop-os/os-release")

        yN = input("Continue? [y/N] ")
        if yN.lower() == "y":
            # Backup /etc/os-release
            os.system("sudo mv /etc/os-release /etc/os-release-backup")
            # Create hard link to /etc/os-release
            os.system("sudo ln /etc/pop-os/os-release /etc/os-release")
    else:
        print("Aborting...")
        sys.exit(1)

I've tried to comment the code as review kind of thing on github, but never did that before. So I think this is a little bit easier for now.

@emilyastranova
Copy link
Contributor Author

I too have never been able to do code revisions in pull requests, been meaning to learn. Will revise

@emilyastranova
Copy link
Contributor Author

With this modification users are still able to continue even though the OS name was never read if they press Y. Is that intended behavior?

@emilyastranova
Copy link
Contributor Author

dist_name will not be initialized and cause issues at elif dist_name in ["arch", "manjaro", "garuda"]:

@bobslept
Copy link
Contributor

No now your Continue? [y/N] is part of the if os.path.exists("/etc/pop-os/os-release"): code.
So if the file exists, you will ask them to continue, if that file did not exists, it will go to the print("Aborting...")

@emilyastranova
Copy link
Contributor Author

I was just looking at that, didn't notice it got indented. Making the change now 👍

@bobslept
Copy link
Contributor

bobslept commented Dec 18, 2021

Great I think we are there! Now we just wait for @AdnanHodzic to review it one last time and then he probably merge it when he is happy!

I will already say thank you for your contribution to auto-cpufreq! 👍

@emilyastranova
Copy link
Contributor Author

Glad I could help, this tool has served me well on my new Framework laptop so I thought I'd give back.

Copy link
Contributor

@bobslept bobslept left a comment

Choose a reason for hiding this comment

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

Well I've just spotted that we do nothing, when we chose N option. So maybe it's a good idea to actually handle it.

auto_cpufreq/core.py Outdated Show resolved Hide resolved
@emilyastranova
Copy link
Contributor Author

Did not mean to hit resolve, let me open the code real quick

@bobslept
Copy link
Contributor

bobslept commented Dec 18, 2021

I don't really know if I did that right. But the thing is: If the user choose N. we need also nicely print abort and exit.

@emilyastranova
Copy link
Contributor Author

Is there a clean way to do that without just adding an identical abort block?

@emilyastranova
Copy link
Contributor Author

    yN = input("Continue? [y/N] ")
        if yN.lower() == "y":
            # Backup /etc/os-release
            os.system("sudo mv /etc/os-release /etc/os-release-backup")
            # Create hard link to /etc/os-release
            os.system("sudo ln /etc/pop-os/os-release /etc/os-release")
        else:
            print("Aborting...")
            sys.exit(1)
    else:
        print("Aborting...")
        sys.exit(1)

Seems a little dirty but it gets the job done

@emilyastranova
Copy link
Contributor Author

image

@bobslept
Copy link
Contributor

Yeah I see. Not really know a nicer way of doing this at the moment. That's why I added another print message. Something like "Operation aborted by user." Would make more sense on that print maybe.

@emilyastranova
Copy link
Contributor Author

That should do it

Copy link
Contributor

@bobslept bobslept left a comment

Choose a reason for hiding this comment

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

What do you think as last change. So actually printing it's doing something after choosing "y"

auto_cpufreq/core.py Show resolved Hide resolved
@bobslept
Copy link
Contributor

bobslept commented Dec 18, 2021

Wow that was fast! Well I think it's great! Thank you very much 👍
And thanks for letting me play with the review option a little bit 🥳

@emilyastranova
Copy link
Contributor Author

Yeah now I understand how the "commit changes" works, just didn't see the indicator for a bit on my side

@AdnanHodzic
Copy link
Owner

Looks great, thank you for your contribution @tyleraharrison and thank you for your assistance reviewing this PR @bobslept!

@AdnanHodzic AdnanHodzic merged commit 7236e67 into AdnanHodzic:master Dec 19, 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.

3 participants