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

Add support for conda envs on windows #52

Merged
merged 9 commits into from
Jan 22, 2019
Merged

Conversation

paddyhoran
Copy link
Contributor

@paddyhoran paddyhoran commented Dec 21, 2018

Hey @konstin,

I have support for building with conda on windows working. I just tested this locally by running pyo3-pack build for the get_fourtytwo example.

I'm assuming that supporting conda on other systems would be pretty much the same but I'm not able to test this.

I'll take a look at updating CI to test this in another PR.

@konstin
Copy link
Member

konstin commented Dec 21, 2018

Thank you for the pull request!

The code looks good. What is not clear to me though is if you want to use conda's base interpreter or the one in the currently activated environment, and if you want to support building for multiple python versions (and if so, do we need to filter duplicates?). For comparison, here's what conda info -e looks like on my machine (Ubuntu 18.04, conda 4.5.11):

# conda environments:
#
base                     /home/konsti/anaconda3
snakes                *  /home/konsti/anaconda3/envs/snakes
snowflakes               /home/konsti/anaconda3/envs/snowflakes

On a side note, I've discovered conda info --json which might be useful as json is a pleasure to use with serde.


The CI failure is because of clippy. You can run clippy locally with

rustup component add clippy
cargo +nightly clippy

Note that you currently have to use clippy on nightly because clippy on stable has some false positives.

@paddyhoran
Copy link
Contributor Author

Hey @konstin,

Thanks for the feedback.

What is not clear to me though is if you want to use conda's base interpreter or the one in the currently activated environment

For my use case, using the interpreter in the active env is sufficient, but I can see the value in building for all envs. For instance, you can easily create a few temp envs in order to build wheels for all the Python versions you need. I could see this really being useful for someone that maintains a package on pypi (which I do not).

I don't think that using the base interpreter is as useful. For instance, I install miniconda only to build envs. I never actually use the base interpreter.

do we need to filter duplicates

Yes, this is an oversight, I'll fix it when I get a chance along with the clippy issues.

@paddyhoran
Copy link
Contributor Author

Hey @konstin,

I addressed your previous comments, please take another look when you get a chance. Let me know how you feel about building for the active env vs all (I favor all, but obviously your call)?

@konstin
Copy link
Member

konstin commented Dec 28, 2018

I finally got to install conda on windows and play around with it.

It'd like to have the output of conda info --envs documented somewhere near the regex parsing it, maybe as an inline comment. I've failed to do that when I added py -0, but it will definitely add this eventually (likely after this pull request is merged, to avoid merge conflicts)

>conda info --envs
# conda environments:
#
base                     C:\Users\Konstantin\Anaconda3
foo1                  *  C:\Users\Konstantin\Anaconda3\envs\foo1
>py -0
Installed Pythons found by py Launcher for Windows
 -3.7-64 *
 -3.6-32
 -2.7-64

Let me know how you feel about building for the active env vs all (I favor all, but obviously your call)?

I like the current solution!

One thing we need is an automated way of testing this, either alongside the other tests or as a standalone script. it should create at least 3 environments, with two having the same version and one a different version, and then check the two versions are picked up correctly, and ideally jsut do nothing if conda is no installed.

src/python_interpreter.rs Outdated Show resolved Hide resolved
@paddyhoran
Copy link
Contributor Author

Hey @konstin,

I updated to use Path::join and I added comments regarding how we find interpreter info, feel free to re-word, etc.

CI was the next step, just wanted to get the functionality in place before I start messing with that :). Do you want to merge this first or should I keep working on CI on this branch?

@konstin
Copy link
Member

konstin commented Dec 29, 2018

Thank you, this is one great doc comment!

The Ci for windows, appveyor, only builds the code and creates binaries for releases. The integration tests always failed for mysterious reasons I couldn't reproduce, so I had to deactivate them. That's why CI for windows isn't important as long as the tests can be run locally.

Do you want to merge this first or should I keep working on CI on this branch?

I'd like to merge after we have a test.

@konstin
Copy link
Member

konstin commented Dec 29, 2018

Oh, and the current travis ci failure seems to be because clippy is broken on nightly rust, which should be fixed by a new rust nightly in the next days.

@paddyhoran
Copy link
Contributor Author

Ok, sounds good, thanks very much.

@paddyhoran
Copy link
Contributor Author

Hey @konstin,

I'm a little stuck on this... My plan is to create a few conda envs (with specific version numbers) and then inspect the target folder to make sure the correct wheels are created.

I think I know how to download conda using powershell and create the environments. However, don't I need a separate job so that the other python interpreters installed for your existing tests don't interfere with what I am trying to test (and the other way around)? I'm sorry, I'm no expert in AppVeyor or Powershell.

Also, which test should I update or replicate for the conda testing?

Thanks

@konstin
Copy link
Member

konstin commented Jan 4, 2019

I'd recommend solving this without appveyor first. For that you can assume that conda is installed and in path. Then just run conda commands as you've said, check that cargo run --quiet -- list-python prints the expected versions. Bonus points for picking one of the test crates, building wheels, installing them in the envs and running check_installed.py, but that's not required.

For the existing interpreters, afaik you can override py by some dummy alias (like here) so that the regex wont capture anything.

Also, which test should I update or replicate for the conda testing?

Once you have a script, you can try to replicate tests/test_integration.rs if you want.

@paddyhoran
Copy link
Contributor Author

As always, thanks @konstin.

@paddyhoran
Copy link
Contributor Author

Sorry for the lack of activity on this, I plan to get back to it soon.

@paddyhoran
Copy link
Contributor Author

@konstin, I'm just testing but AppVeyor did not trigger even though I made a change to AppVeyor.yml?

@konstin
Copy link
Member

konstin commented Jan 16, 2019

I've reactivated appveyor; It should build on the next push. The code looks good, I'll try testing it tomorrow.

@paddyhoran
Copy link
Contributor Author

Ok, sounds good, thanks. Still a WIP but just wanted to how it fairs with CI.

@konstin
Copy link
Member

konstin commented Jan 18, 2019

The tests look good, but need one addition: We need to check the selected interpreters are actually the conda environments, e.g. by checking that pyo3-build-env- is in the interpreter path. Otherwise it could happen that the conda detecting code becomes broken and the test still passes because it chooses the system python versions. (You can confirm that by running the test in an environment where conda isn't in PATH after placing a dbg!() around wheels in for (filename, _, python_interpreter) in wheels.


I've tried to get the windows tests working on appveyor in #67, but had only moderate success. I think the best is to merge once we have the additional and leave figuring out appveyor for later.

@konstin
Copy link
Member

konstin commented Jan 22, 2019

Thank you!

@konstin konstin merged commit 7cd4d24 into PyO3:master Jan 22, 2019
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