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

switch python command according to os #16

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

angusmcb
Copy link

@angusmcb angusmcb commented Sep 3, 2024

Another fix for #5

@angusmcb
Copy link
Author

angusmcb commented Sep 3, 2024

I checked it on my ubuntu and windows machines, but needs testing with a variety of setups

@@ -251,9 +251,13 @@ def pip_install_reqs(self, reqs_to_install):
os.makedirs(self.prefix_path, exist_ok=True)
log(f"Will pip install {reqs_to_install}")

# python is normally found at sys.executable, but there is a bug on windows qgis so use 'python' instead
# https://github.com/qgis/QGIS/issues/45646
python_command = 'python' if os.name == 'nt' else sys.executable
Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure, is this tested?
I guess the python command is on PATH, so it looks plausible.

Copy link
Author

Choose a reason for hiding this comment

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

I tested it on my machine and it worked on Windows. However, it would be good if it could be tested by others as it could depend on how the machine is setup (I have another installation of python on my machine, for example).

Copy link
Member

Choose a reason for hiding this comment

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

You could print sys.executable from within the python subprocess to validate which one is started?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python_command = 'python' if os.name == 'nt' else sys.executable
python_command = "python" if os.name == "nt" else sys.executable

let's make our linter happy too

@m-kuhn
Copy link
Member

m-kuhn commented Sep 3, 2024

Thanks!

@angusmcb
Copy link
Author

I just tested this on a fresh install of qgis on a fresh windows machine. Whilst this change works, and python is found, pip seems to not be installed by default !

Should I open a new issue for this?

@m-kuhn
Copy link
Member

m-kuhn commented Sep 19, 2024

I just tested this on a fresh install of qgis on a fresh windows machine. Whilst this change works, and python is found, pip seems to not be installed by default !

Should I open a new issue for this?

This is surprising, I've seen many scripts rely on it being installed and I've never seen any reference for how to install this manually. Are you sure it is picking up the python installation that comes with QGIS?

@angusmcb
Copy link
Author

Actually this was my fault - I used the osgeo4w installer on 'advanced' - when installed using the normal express install pip is included.

@m-kuhn
Copy link
Member

m-kuhn commented Sep 19, 2024

So, let's merge?

@angusmcb
Copy link
Author

yup :-)

@m-kuhn m-kuhn merged commit 2d6665e into opengisch:main Sep 19, 2024
1 of 2 checks passed
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.

2 participants