python binary resolution

[MitchellBerend]

Resolves #241

This pr aims to add a way to find the location of the python binary. It checks all directories defined in the $PATH environment variable and returns the first of the following list it finds.

• python3
• python
• python2

[MitchellBerend]

@cnpryer Can you give this a quick glance? I thought the src/huak/env/system.rs file was a good location to put the logic for finding the python binary.

This pr only exposes a single function there called find_python_binary_paths. I don't see a good way to write a test for this function though, since it gets its information from the PATH environment variable.

[cnpryer]

At first glance it looks good. I can come back to this later today or tomorrow. I do wonder if there's a way to test it. I mean a really dumb solution would be to just mock the PATH var and pass the string to the function (changing the function signature to do so). But I haven't thought about it much.

I really don't mind even dumb tests like this because it at least provides some form of coverage. You never know if a refactor or some other change might impact future implementations in unexpected ways.

At the end of the day I don't mind if we don't test this right now.

[cnpryer]

TODO: Is Venv a trait of Env?

This is part of the refactor I mentioned. At the moment I'm thinking I'll refactor to an Env enum composing different environment implementations for Project such as the Venv struct with the PythonEnvironment trait. I need to think about it more though.

[MitchellBerend]

I had a test that checked if the paths separated by a : were split correctly but it looked kind of pointless.

As for the public function, something like a set test_files/usr/bin dir can be added with empty files.

[cnpryer]

I had a test that checked if the paths separated by a : were split correctly but it looked kind of pointless.

Well I meant more about the logic you've implemented. So you're searching for python3 then python then python2. You're also searching first-to-last. I think that's behavior we can test to ensure it remains constant.

As for the public function, something like a set test_files/usr/bin dir can be added with empty files.

Any reason not to just bake a tempdir?

[MitchellBerend]

Well I meant more about the logic you've implemented. So you're searching for python3 then python then python2. You're also searching first-to-last. I think that's behavior we can test to ensure it remains constant.

Ah I read that the wrong way then.

Any reason not to just bake a tempdir?

I usually just create files by hand, so all the other parts of the function are as similar as possible.

Edit: I'm also not familiar with tempdir but I can definitely use that too

[cnpryer]

I usually just create files by hand, so all the other parts of the function are as similar as possible.

Makes sense. It also could just mean we need to think about different function signatures. For example you could bake a tempdir with the files and directories you want. Then pass the path to the directory to the function, designing the function to hand that responsibility off up to the caller.

If you do decide to just create files feel free to use the resources directory. I do think tempdir is more often preferred, but I'm open to ideas.

[MitchellBerend]

It also could just mean we need to think about different function signatures.

I would like to have it so the function can just be called and it will return whatever binary matches first. Otherwise there is some processing in the Venv class itself.

[cnpryer]

I would like to have it so the function can just be called and it will return whatever binary matches first. Otherwise there is some processing in the Venv class itself.

What about just abstracting the actual search into another function and having that require a from_dir: Option<Path>?

Also some additional feedback: The rest of the project should be implementing top-down function order. I think you're doing this bottom up. I may have missed that with the last PR, but could we reorder those?

pub find_python_binary_paths() ...

parse_path() ..

find_binary(...) ...

search_python_versions(...) ... // or a better name

test search_python_versions() ...

Also should it be find_python_binary_paths or find_python_binary_path?

[MitchellBerend]

Also some additional feedback: The rest of the project should be implementing top-down function order. I think you're doing this bottom up. I may have missed that with the last PR, but could we reorder those?

Sure.

What about just abstracting the actual search into another function and having that require a from_dir: Option?

Just to be clear, do you mean like this?

def  find_python_binary_paths(path=None):
	if not path:
		# use $PATH

[cnpryer]

Just to be clear, do you mean like this?

def  find_python_binary_paths(path=None):
	if not path:
		# use $PATH

Yea or just pass an iterator so its paths or something. Doesn't have to be a string to break up. Just an iterable sequence of paths. That'd probably be more rust-idiomatic.

Edit: This feedback is for the abstracted search logic referring to the python version searching mentioned earlier. You still break the PATH up in the top-level find function you have. I'm cool with whatever you decide if we make that logic testable.

[cnpryer]

Hey let me know when you'd like for me to check this out. I'd like to start mocking directories when we can, so we should use tempdir here. If you want I can help you out with that when you're ready.

[MitchellBerend]

Hi sorry I meant to response to this yesterday but I forgot. I implemented the hand rolled files first to see what the difference would be between that and the tempdir I plan to also implement that but I have not found the time to work on this more.

[cnpryer]

All good. I'm happy to roll the rest of the PR together. Just give me the green light. I don't want to step on your toes.

[MitchellBerend]

I think it's worth exploring all options, especially since this is in the early stages of this project.

[cnpryer]

I think it's worth exploring all options, especially since this is in the early stages of this project.

I'm not sure I follow. To create a test to always ensure the search behavior is consistent we can add targets to a temporary directory for the test. Let me know if you need help with that.

[MitchellBerend]

I'm happy to roll the rest of the PR together.

I thought you meant that this test is fine the way it is. I wanted to also implement the tempdir to see what the differences are.

[MitchellBerend]

How would I go about actually creating a tempdir that contains either one, none or multiple of the files that match the search pattern? Wouldn't that be the same as hand rolling the structure?

[cnpryer]

This may not be complete, but I can revisit this later.

#[cfg(tests)]
mod tests {
    fn test_python_search_windows() {
        let directory = tempdir().unwrap().into_path().join("mock-directory");
    
        fs::write(directory.join("python.exe", "")?;
    
        assert_eq!(search(directory)?, direcotry.join("python.exe"));
    }

   fn test_python_search_macos() {
        let directory = tempdir().unwrap().into_path().join("mock-directory");
    
        fs::write(directory.join("python", "")?;
    
        assert_eq!(search(directory)?, direcotry.join("python"));
    }

    ...
}

[cnpryer]

Looking good. Are we walking from_dir at all? So in other words, are we recursively searching from_dir?

[MitchellBerend]

Looking good. Are we walking from_dir at all? So in other words, are we recursively searching from_dir?

No I'm not, is this the norm because I always thought these dirs essentially only had binaries.

[cnpryer]

If it's a venv dir it won't work

[MitchellBerend]

Should I make find_binary recursively look into all sub dirs, if so breadth or depth first?

If I drop into a virtual env I can still just echo $PATH and the first will contain the sym links to the python binaries.

[cnpryer]

We can do something simple for now and just walk the directory for each sub dirs, searching at most something like 3 steps deep from the root dir.

Can always come back and implement something more robust. Lmk if you want me to help you wrap this up.

[MitchellBerend]

I think the current implementation search all dirs in the paths, I think a test for walking the path would be good here too. I wouldn't mind some help with that.

[cnpryer]

Hey sorry for the inactivity. Things are a bit busy with my day job. Hoping to re-balance sometime after this PR. I'll see if I can knock this out today.

Thanks again for contributing!

[cnpryer]

think a test for walking the path would be good here too

If you mean baking a path variable with directories to walk, i agree that'd be nice, but I'm fine with just baking a directory to walk for now. No need to get bogged down here. I'll add a recursion limit until we can come back to this and polish.

[cnpryer]

I made a TODO comment to revisit how we're using this. It's nice to search PATH, but thinking about it now it's kind of redundant if python is added to the PATH since it'd be an available alias anyway. This will be nice though if we want to use it for systems that don't have it added to PATH already.

[cnpryer]

Rip. I'll create a separate PR to figure out the maturin action issue. Looks like there was broader update to it recently.

[cnpryer]

I removed the recursion limit because it was incomplete and i didn't want to just butcher everything. We can come back to it.

Also I'll merge this regardless of the maturin issue and follow up at #351

[cnpryer]

Looking into the windows issues I think we're just not handling the environment variable parsing properly there. I'll look into it after this PR. For now I'll just have it default to attempt the alias if it cannot get it from the path variable.