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 frozen environment support #238

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AlexPaiva
Copy link

Fixes #237

Fixes pytest-dev/pytest-xdist#991

Adds support for frozen environments such as pyinstaller, with proper imports on the pyinstaller side this change will work, tested with pytest and pytest-xdist.

Extra documentation available on both the mentioned issues.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Any idea how we could test this in automation, it may easily break if we have no test for it

@AlexPaiva
Copy link
Author

Any idea how we could test this in automation, it may easily break if we have no test for it

Yes, let me explain how it would differenciate for regular testing:

For a 'normal' run (meaning it would run perfectly without this PR and with the PR also works perfectly still) just create any test you want in a separate file (insert prints or write to files inside of them so you can debug if they ran or not), then pip install pytest xdist then start from a main.py file a call to pytest.main such as: pytest.main(["-n","2",relative_to_assets('test.py')]) where relative_to_assets() is a function that returns the correct path depending if it's a frozen env or not:

def relative_to_assets(relative_path):
    try:
        base_path = sys._MEIPASS
    except Exception:
        base_path = os.path.dirname(__file__)
    return os.path.join(base_path, relative_path)

Run via command line as usual with python main.py It should work as it normally would.

The issue then arises when you compile into a pyinstaller single .exe where it no longer works, the expected debug print/file write will not happen and errors will arise, usually it opens a second .exe instead (related to trying to open sys.executable I assume). Basically just open the .exe and it should execute the code of main.py and it does but the pytest.main'' call doesn't execute properly as you will be able to test yourself.

For a run with this PR if you run normally it will still work without any kind of issues and the difference comes when ran with the pyinstaller single .exe, it will work perfectly aswell just by changing the execnet source code with this PR and without making any other changes.

To compile the main.py and according files properly into an .exe you will need this main.spec or similiar configuration for pyinstaller (assuming the main.spec and main.py are in the same directory, replace the execnet path in the .spec file with the path to the source code of the installed execnet on the test machine at the C:/Users/AA/AppData/Local/Programs/Python/Python310/Lib/site-packages/execnet path located in the datas), download all the files here: https://we.tl/t-tYpHw527Kw

On there the pytest-xdist folder is just a copy of: https://github.com/pytest-dev/pytest-xdist/tree/master/src/xdist so feel free to download yourself if you want.

On that directory run python main.py to test it normally, then run pyinstaller main.spec to build the .exe, then on the dist folder that is created should be a main.exe, just start it and a console will open and run the test. If you have the original execnet it should give errors or not run, if you have my PR it will run without any issues. Inside the test.py feel free to add any other tests you want.

Let me know if you need any extra details.

@nicoddemus
Copy link
Member

@AlexPaiva I think @RonnyPfannschmidt just meant that we need to add tests to execnet to ensure the behavior this PR is adding does not break in the future.

For that, we need some tests:

  • We just need to spawn a sub interpreter and make sure the EXECNET_SPAWNING_SUBPROCESS is set appropriately.
  • I would say the same about PYTHON_EXECUTABLE, but it is not clear to me if we need this variable at all.

@@ -18,6 +18,9 @@
class Popen2IOMaster(Popen2IO):
def __init__(self, args, execmodel):
PIPE = execmodel.subprocess.PIPE
if is_frozen_environment():
args[0] = str(sys.executable)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct, we should not be mutating the input arguments like this, nor assuming the first argument is the interpreter. This class is public, and even if it was not the case, this is certainly surprising behavior.

Clients of this class should instead use sys.executable instead of just "python", for example here: https://github.com/AlexPaiva/execnet/blob/227da4174e349b0cc198619b5fd186c6f137df82/src/execnet/gateway_io.py#L86C5-L86C43

def get_python_executable():
"""Get the correct Python interpreter path."""
if is_frozen_environment():
return os.environ.get("PYTHON_EXECUTABLE", "python")
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned, not clear to me why this is needed, seems to me we should always return sys.executable from here?

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Besides my comments, I think we should at least document things a bit, even if we decide to mark this workflow as experimental.

@AlexPaiva
Copy link
Author

Sorry on the late reply, was very busy!
You mean, why not use sys.executable directly correct?

@nicoddemus
Copy link
Member

You mean, why not use sys.executable directly correct?

Yes. 👍

@AlexPaiva
Copy link
Author

The goal was to leave the entire thing as it was and only change when it's a frozen env, especially for testing.
But agreed that it can be simplified.

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.

pyinstaller support Not working with Pyinstaller
3 participants