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

Make x tool work with MS store's installed python #105786

Closed

Conversation

albertlarsan68
Copy link
Member

This may introduce problems on other OSes where the python installation is broken, but this shouldn't happen.

@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2022

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2022
src/tools/x/src/main.rs Outdated Show resolved Hide resolved
@ChrisDenton
Copy link
Member

Have you confirmed this works? The way the Store does application links is frustratingly odd. It does not use symlinks, it uses a special Application Execution Alias that only works for CreateProcess and is utterly messed up otherwise. And the stdlib does not currently have any special handling for them so I'm surprised that this doesn't error.

@albertlarsan68
Copy link
Member Author

I use this exact change now, and it works wonderfully.

@ChrisDenton
Copy link
Member

Oh hm, looking at the code, does this not just always return "python" unless a filesystem error occurs? Surely this breaks in the case where e.g. "python" does not exist but "python3" does?

@albertlarsan68
Copy link
Member Author

albertlarsan68 commented Dec 17, 2022

The old behavior was "find any file, directory or live symlink named python, python3 or python2". The new behavior would allow this and the broken symlinks, which is how Rust code sees the aliases made for the Microsoft Store's Python. So if you don't have access to any of those named according, it won't find them.

@ChrisDenton
Copy link
Member

I mean the new code is reading paths from the PATH environment variable. Let's say, for the sake of argument, the first path is C:\Windows. There is no python.exe in C:\Windows so try_exists will return Ok(false). This means that the python() function will return python even though it doesn't exist. And of course it would return the same thing if it does exist. So you could get the same effect by simply replacing the python() function with one that always returns the string "python".

@albertlarsan68
Copy link
Member Author

No, it will return Err(NotFound). It only returns Ok(false) if the symlink's target is not available.

@ChrisDenton
Copy link
Member

Ah, the documentation is unfortunately confusing but that's not what it means. It will return Ok(false) if the file is not found and because it follows symlinks it will also return Ok(false) if the link is broken. You can see this is the example. Or you can try it in the Rust Playground.

@albertlarsan68
Copy link
Member Author

I hope this works, but I just thought of another way to find which python to choose: try to call each of them with --version, and keep the first one to return successfully.

There is still the problem with the broken symlinks, but I think this is a small price to pay to make this even more universal.

@Mark-Simulacrum
Copy link
Member

r? @jyn514

I seem to recall us having code like this (with --version) which caused dialogs to pop up for folks asking to install Python even though they already had one. But maybe I just dreamed that up :)

@rustbot rustbot assigned jyn514 and unassigned Mark-Simulacrum Dec 17, 2022
@jyn514
Copy link
Member

jyn514 commented Dec 17, 2022

I would like to avoid making major changes to the x tool unless they're to use the x.ps1 or ./x entrypoints. That would let us delete this python logic altogether. cc #104350 (review)

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2022
@albertlarsan68
Copy link
Member Author

Mark-Simulacrum:
It is the case by default, until you disable the aliases or install a Store python

@jyn514:
Using the x tool is the only way on Windows to bootstrap, unless you're willing to type python .\x.py every time, when not using Powershell. I use Nushell, and it is a major usability bonus, as simply typing .\x.py will open in a new console, and .\x.ps1 will open Notepad by default.

@jyn514
Copy link
Member

jyn514 commented Dec 17, 2022

@albertlarsan68 yes, I understand. I'm suggesting that we keep the x binary, but have it run the shell scripts instead of trying to find python in a fourth place.

@albertlarsan68
Copy link
Member Author

Well, I just have to do that then.

@albertlarsan68
Copy link
Member Author

PR is up at #105844

@jyn514
Copy link
Member

jyn514 commented Dec 17, 2022

Thanks!

@jyn514 jyn514 closed this Dec 17, 2022
@albertlarsan68 albertlarsan68 deleted the x-tool-with-store branch December 18, 2022 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants