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

Ensure correct sys.path contents in Windows, for sub-processes launched by Mu #726

Merged
merged 2 commits into from Jan 10, 2019

Conversation

ntoll
Copy link
Member

@ntoll ntoll commented Dec 21, 2018

This PR introduces three changes.

  • A fix for Unable to import file from mu_code folder (Windows) #697, mu_code not included in sys.path on Windows 10 #662 so the expected / correct paths are included when Python 3 mode launches / debugs a user's script. This works by attempting to drop a mu.pth file in the directory Python/Windows reports as the USER_SITE (a directory that's usually writeable by the user, but which Python checks for configuration files when it starts up). If this step fails, Mu logs the error and continues as it used to behave (but the user will thus encounter the import errors reported in the referenced issues). The problem was caused by the version of Python used by the Windows installer for Mu running in "isolated" mode (i.e. protected from other versions of Python that may be installed on the system).
  • A fix for Debugger Failes if numpy is importet #612 and Debug not working with requests module #581 so the path to pythonXX.zip (where XX is Major/Minor numbers of the Python version) is included in the sys.path. This is simply an injection of the location of python36.zip into sys.path before the debugger kicks off the script-to-be-debugged.
  • A minor fix to ensure the zoom-level from the last session is restored on application start. This is a trivial update.

While a relatively small PR, the path related issues took a while to figure out since there were various (and ultimately unusable) potential approaches which needed to be checked before I could be sure I had a "good" solution. I believe the approach taken here meets both the "it's simple" and the "it works" heuristics. However, I'm painfully aware that I'm no Windows expert nor do I have a Windows machine available to me which would allow me to check some of the more "weird" configurations Mu is likely to encounter (especially in school based environments).

As a result, I'd love some feedback from Windows-related folks before merging. :-)

cc/@tjguk and @tim-mccurrach

@tim-mccurrach
Copy link
Contributor

tim-mccurrach commented Dec 22, 2018

With regard issues #612 and #581 everything worked well in all of the situations I tried.

With regard importing a module for mu_code I found that running a development version inside a virtualenv, everything worked well in both debug mode and running the file as normal. However, when I ran make win64 and tried the same thing on the version I installed, I found that in debug mode I could import from mu_code, but when I ran the code normally I could not. (Both the script I was running and the module I tried to import were in mu_code).

I'm not exactly sure why this is the case. Printing out sys.path in each of the various situations gave the following:

  • In the dev version inside the virtualenv, the only difference in sys.path was that in debugger mode there was an additional path ...lib\site-packages\IPython\extensions

  • In the installed version, again the only difference between debug and normal mode was the additional IPython path.

  • mu_code was in sys.path for the dev version.

  • In the installed version mu_code was not in sys.path, for debug or normal. Although I could import from mu_code in debug mode.

I'm sorry this probably isn't the best news for your Saturday morning. If there's anything you want me to try on my machine just let me know.

EDIT: Trying the same thing with file in other locations: The installed version will import a file from the directory of the file being run in debug mode only, so it seems like something about the way files are imported in debug mode includes the current directory, but not when run normally. (Everything still works fine in the virtualenv version).

@ntoll
Copy link
Member Author

ntoll commented Dec 22, 2018

@tim-mccurrach can you run the following script and copy the full output into a comment on this issue. Can you also attach your log file too please..?

import sys
import os
import site

for p in sys.path:
    print(p)

print(site.ENABLE_USER_SITE)
print(site.USER_SITE)
print(os.path.exists(os.path.join(site.USER_SITE, 'mu.pth')))

Thank you!

@tjguk
Copy link
Collaborator

tjguk commented Dec 22, 2018

@ntoll Just from a quick perusal of the code... At least on my system, site.USER_SITE doesn't exist by default. (You can actually see this if you run python -msite). And AFAICT we're not creating it. I'm actually surprised we're not seeing some kind of filesystem error.

I'll try to have more or a look later today or tomorrow

@tim-mccurrach
Copy link
Contributor

Running the installed version of v1.0.2 I get the following output:

C:\Users\timmc\AppData\Local\Mu\Python\python36.zip
C:\Users\timmc\AppData\Local\Mu\Python
C:\Users\timmc\AppData\Local\Mu\pkgs
True
C:\Users\timmc\AppData\Roaming\Python\Python36\site-packages
False

When running in debugger mode:

c:\users\timmc\mu_code   # Note: This is the location of the file, and won't necessarily be mu_code
C:\Users\timmc\AppData\Local\Mu\Python
C:\Users\timmc\AppData\Local\Mu\pkgs
C:\Users\timmc\AppData\Local\Mu\pkgs\IPython\extensions
c:\users\timmc\appdata\local\mu\python\python36.zip
True
C:\Users\timmc\AppData\Roaming\Python\Python36\site-packages
False

Last part of log file attached:
logs.txt

@tjguk
Copy link
Collaborator

tjguk commented Dec 23, 2018

Well that was an interesting exploration into the somewhat messiness ofo the site/isolation flags. I was particularly confused by the fact that the virtual environment I was working in for development turned off
user site support, while the NSIS-packaged Python didn't. While I had expected the opposite.

In case it helps anyone, I've been using this code to try out different environments:

https://gist.github.com/tjguk/babef6799c87b3a5e2c240ca8add9c45

The business with the "USERPROFILE" is because I use a different user for installed as opposed to development Mu.

Anyhow, the long and short seems to be that adding a simple os.makedirs(site.getusersitepackages(), exist_ok=True) is likely to set things right. I'll experiment a bit more but I'll probably just mod the PR with that.

@ntoll
Copy link
Member Author

ntoll commented Dec 23, 2018

@tjguk @tim-mccurrach Aha... this feedback is invaluable, and I think I can see what the problem is.

@tim-mccurrach the reason it's not working for you is because the mu.pth file isn't created so the runner and debugger child processes don't pick it up and add the paths that are supposed to be found therein into sys.path for the child process. That's obvious from your log.txt file (see the bit which says, [Errno 2] No such file or directory: 'C:\\Users\\timmc\\AppData\\Roaming\\Python\\Python36\\site-packages\\mu.pth'). This also chimes with @tjguk's comments too -- the USER_SITE directory doesn't exist so where's the filesystem error? (It's in the logs).

So here's where I've slipped up. I've used the site.USER_SITE constant to get the path I need. However, what I should be doing is calling the site.getusersitepackages function (see: https://docs.python.org/3/library/site.html#site.getusersitepackages) which will both return the correct path and, as the documentation tells us, will also create the directory if it doesn't already exist. The reason the mu.pth file isn't written on your machines is probably (my hunch) because the directory path hasn't been created. Why does it work on my machine? I think it's because I was experimenting with the site module in the REPL and must have called the method, thus creating the directory, before settling on the solution I pushed as this PR.

Ergo, the simplest solution is to replace assignments from the site.USER_SITE to the return value of site.getusersitepackages.

I'll push that change later today. Please re-test. :-)

@tjguk
Copy link
Collaborator

tjguk commented Dec 23, 2018

Just updating after a quick conversation with @ntoll elsewhere.

In short: he'd misunderstood the question of whether site.getusersitepackages creates the folder (it doesn't -- it only populates site.USER_SITE if it needs to). So my os.makedirs suggestion seems like the way to go.

@ntoll
Copy link
Member Author

ntoll commented Dec 23, 2018

@tjguk @tim-mccurrach OK... thanks to Tim for the heads up on the actual behaviour needed. I've just pushed a small change to fix this (and updated the tests).

Please test!

I'm off to play Christmas carols with the local brass band and will catch up later today... 🎺 🎵 🎄 👍

@bddicken bddicken mentioned this pull request Jan 6, 2019
@ntoll
Copy link
Member Author

ntoll commented Jan 10, 2019

@tjguk @tim-mccurrach could you please confirm things work with this PR? It's one of the final blockers for a 1.0.2 release. Thanks.

@tjguk
Copy link
Collaborator

tjguk commented Jan 10, 2019

Sorry; setting time aside this evening

@ntoll
Copy link
Member Author

ntoll commented Jan 10, 2019

Thank you!

@tjguk
Copy link
Collaborator

tjguk commented Jan 10, 2019

Ok; I can confirm that, on an installed version of Mu, I was able to import from another module within the mu_code directory.

@tjguk
Copy link
Collaborator

tjguk commented Jan 10, 2019

... and that, if I load a file from another directory (ie not mu_code), then I am able to import files from that directory as well as from the mu_code directory.

@ntoll ntoll merged commit ece35c3 into master Jan 10, 2019
@ntoll
Copy link
Member Author

ntoll commented Jan 10, 2019

Thanks for the heads up. Merging for 1.0.2 release.

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.

None yet

3 participants