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

Simplify virtualenv test in test_testing #374

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

YannickJadoul
Copy link
Member

@YannickJadoul YannickJadoul commented Jun 13, 2020

I'm curious if this will allow us to get rid of the __CIBW_VIRTUALENV_PATH__ environment variable we always set in the test virtualenv, with the only purpose of using it in our tests.

It's not brilliantly documented, but it seems a reasonable assumption and if anything changes in future virtualenv releases, this is much more likely to give false negative detections (and thus fail tests when they should not, rather than vice versa).

See #283

@YannickJadoul
Copy link
Member Author

Ah, yes, if the CI passes: @Czaki, what do you think of this? Could this replace #283, or do you have objections?

@Czaki
Copy link
Contributor

Czaki commented Jun 14, 2020

@YannickJadoul I remember that I try with base_prefix, etc and do not find working one. Maybe some upgrades in virtualenv will fix it.

Other option which i see Is that PATH variable is controlled by cibuildwheel. So maybe:

base_path = os.environ.get("PATH").split(os.path.pathsep)[0]

@YannickJadoul
Copy link
Member Author

@YannickJadoul I remember that I try with base_prefix, etc and do not find working one. Maybe some upgrades in virtualenv will fix it.

@Czaki, yup, it seems like it now does work in virtualenv? The only failure is the problem with the PyPy linking (see #371).

Other option which i see Is that PATH variable is controlled by cibuildwheel. So maybe:

base_path = os.environ.get("PATH").split(os.path.pathsep)[0]

No, I think your original solution was better then. Here you're just testing the same as what the code does, so I don't think it would make a great test.

But anyway, now this solution with base_prefix works, do you think we should merge it to replace the old test?

@Czaki
Copy link
Contributor

Czaki commented Jun 14, 2020

I think this could be replaced.

@YannickJadoul
Copy link
Member Author

Alright, thanks, @Czaki!

Up to @joerick to check then, but this could be merged.

@joerick
Copy link
Contributor

joerick commented Jun 16, 2020

Nice bit of cleanup! :)

@joerick joerick merged commit df07aaf into pypa:master Jun 16, 2020
@YannickJadoul YannickJadoul deleted the detect-virtualenv-real_prefix branch June 16, 2020 15:15
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