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

Fix a problem with the way we were calling virtualenv #2145

Closed
wants to merge 20 commits into from

Conversation

zurgeg
Copy link

@zurgeg zurgeg commented Apr 20, 2020

This PR fixes a problem where any buildozer build errors with Can not import virtualenv.__main__; virtualenv is not a package as I tested and when I made this modification it worked.

@zurgeg
Copy link
Author

zurgeg commented Apr 20, 2020

Just testdrove the PRs changes on my local build machine, seems to work.

@AndreMiras
Copy link
Member

Thanks you @zurgeg for the initiative.
The CI is failing /home/user/app/venv/bin/python3: No module named virtualenv --python=3.
We're probably just missing the system dependency. This is not yet clear to me why as the module is part of the setup.py which we pip install -e from the Makefile.
This needs to be investigated/fixed before we can merge

@zurgeg
Copy link
Author

zurgeg commented Apr 20, 2020 via email

@zurgeg
Copy link
Author

zurgeg commented Apr 20, 2020

I think it is caused by there being 2 different versions, venv in >=Python 3.6 and virtualenv in <Python3.6 (I think)

@zurgeg
Copy link
Author

zurgeg commented Apr 21, 2020

Finally just got those builds to work (I think). It had to do with a single whitespace on a line.

@zurgeg
Copy link
Author

zurgeg commented Apr 21, 2020

For some reason the try: except: block is not trying...

@zurgeg
Copy link
Author

zurgeg commented Apr 24, 2020

@AndreMiras turns out my first build was lucky... I just experienced your same issue, changed python3 to virtualenv (Should work anywhere), and it started working.

@opacam
Copy link
Member

opacam commented Apr 24, 2020

@zurgeg ,

For some reason the try: except: block is not trying...

Try changing:

ModuleNotFoundError by sh.ErrorReturnCode_1

@AndreMiras
Copy link
Member

Thanks for investing time into this as I haven't had time lately.
Why can't we make venv being a p4a dep using setup.py then rather than hoping for it to be available?

@opacam
Copy link
Member

opacam commented Apr 24, 2020

mmm...I think that venv is already integrated into python standard modules (since v3.3)... so is not necessary to touch setup.py right?

@zurgeg
Copy link
Author

zurgeg commented Apr 24, 2020

Oh yeah, I'll do that, also doing what I think works for all Python versions.

@zurgeg
Copy link
Author

zurgeg commented Apr 24, 2020

Finally got those builds to work! So apparently no matter if you use venv or virtualenv you can still use the virtualenv command.

@AndreMiras
Copy link
Member

But wait now you removed the --python {version} parameter and only using plain virtualenv it means system will pick the default Python version which can be 2.7 (e.g. Ubuntu 18.04) right? Also I can't think of how this is solving the original problem on buildozer you mentioned 🤔

@zurgeg
Copy link
Author

zurgeg commented Apr 24, 2020

Oh... Hold on, I though it was --python=..., so I thought that was problematic, hold on...

@AndreMiras
Copy link
Member

AndreMiras commented Apr 24, 2020

Also you know you can try things locally e.g. using Docker so you don't have to wait for the CI which is probably slower than your local build.
Edit: last thing you can learn how to squash commits next that's not a must (as we can do it upon merging), but definitely a nice thing to know/use

@zurgeg
Copy link
Author

zurgeg commented Apr 24, 2020

Just fixed your issue @AndreMiras!

partition(".")[0]
),
'venv'
'-p python{}'.format(
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly this didn't change, but we only changed venv = sh.Command('virtualenv') then maybe don't introduce a change here if we don't end up fixing that line. I mean rollback the full arg --python= and the indentation so it's clear what the actual fix is

Copy link
Author

Choose a reason for hiding this comment

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

Okay, doing that now...

Copy link
Author

Choose a reason for hiding this comment

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

For some reason it gives path =python3 not found from --python=python3.

@zurgeg
Copy link
Author

zurgeg commented Apr 25, 2020

Also @opacam I don't think using venv is a good idea because otherwise we would be dropping support for under 3.3, even though 3.3<= isn't used much, it isn't a good idea to just cut it.

@zurgeg
Copy link
Author

zurgeg commented Apr 25, 2020

Okay, going way back to my try... except block idea... I think I got it to work thanks to @opacam, it should provide optimal capability (especially because Python 2 still works) with older versions!

@AndreMiras
Copy link
Member

This is properly painful right? Honestly we don't mind much about Python 3.3, it's not like it wasn't already a nightmare to support 3.6+ and multiple platforms.
opacam made a not bad attempt to ditch virtualenv in his on-going PR and this should also (hopefully) simplify code/dependencies #2152

@zurgeg
Copy link
Author

zurgeg commented Apr 25, 2020 via email

@AndreMiras
Copy link
Member

Thanks for bring this up to our attention and helping out debugging this complex one ♥️

@inclement
Copy link
Member

Looks like this was closed by merging #2152, but thanks @zurgeg for the PR.

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.

4 participants