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

Installing on Windows raises a PermissionError against the registry #682

Closed
tjguk opened this Issue Oct 21, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@tjguk
Copy link
Contributor

tjguk commented Oct 21, 2018

This is a placeholder issue until I can reproduce it but, from memory:

Win10 Pro 17134
64-bit 1.0.1 installer from codewith.mu
Installed with default settings
During the installation messages I noticed a PermissionError exception relating to a registry key.

At the time I was with a bunch of Year 8s and was moving fairly quickly so I didn't pick up the message. I'll look for NSIS logs and/or reproduce. There were no apparent problem actually running Mu afterwards, although we were only doing very simple things

@tjguk tjguk self-assigned this Oct 21, 2018

@tjguk

This comment has been minimized.

Copy link
Contributor Author

tjguk commented Oct 21, 2018

mu-install.log

Logs attached from a fresh user install

@tjguk

This comment has been minimized.

Copy link
Contributor Author

tjguk commented Oct 21, 2018

Ok; so the offending code is in a file which is generated by the Pynsist packaging. If you've built, you can see the code here: mu\build\nsis_system_path.py:add_to_system_path

It seems that the function is being called without specifying whether it's an all-users install or not. The function defaults to True so the code will attempt to open the system Environment reg key for writing -- which won't be allowed.

It look as if this is an upstream bug in pynsist so I'll look for somewhere to report it to there. Meanwhile, the only fallout from our point of view is that Mu's location isn't added to the PATH env var -- which is unlikely to affect anyone badly.

@tjguk

This comment has been minimized.

Copy link
Contributor Author

tjguk commented Oct 21, 2018

Looks like this is takluyver/pynsist#167

@takluyver

This comment has been minimized.

Copy link

takluyver commented Oct 30, 2018

I've had a go at fixing this in Pynsist; if you have time to test with it, that would be welcome:

takluyver/pynsist#170

@carlosperate

This comment has been minimized.

@takluyver

This comment has been minimized.

Copy link

takluyver commented Oct 30, 2018

Thanks @carlosperate, nice use of CI :-)

@tjguk

This comment has been minimized.

Copy link
Contributor Author

tjguk commented Nov 3, 2018

I can confirm that the x64 installer works ok. If I get a chance I'll try the 32-bit but I don't imagine there's a problem. Thanks for doing those, @carlosperate. And thanks for the fix, @takluyver .

AFAICT we're not vendoring the pynsist code with mu, so it looks as though the actions are that maintainers install the updated pynsist and that @carlosperate branch be merged so Appveyor etc. pick up the right version of pynsist.

@takluyver

This comment has been minimized.

Copy link

takluyver commented Nov 3, 2018

Thanks @tjguk. Just to check, did you try both installing and uninstalling with both a user install and an admin install?

I think @carlosperate's branch is only needed for this testing. Once we're happy with the fix, I'll make a new Pynsist release, and you should benefit from it automatically the next time you build Mu. It doesn't look like you're pinning Pynsist, so you shouldn't need to do anything.

@carlosperate

This comment has been minimized.

Copy link
Member

carlosperate commented Nov 3, 2018

Yep, the CI builds will install the latest version from the requirements.txt file, so when a new version is released it will start using that one.

@takluyver

This comment has been minimized.

Copy link

takluyver commented Nov 6, 2018

OK, released Pynsist 2.3 - you can hopefully close this.

@tjguk

This comment has been minimized.

Copy link
Contributor Author

tjguk commented Nov 11, 2018

Fixed upstream. Thanks @takluyver

@tjguk tjguk closed this Nov 11, 2018

@tjguk tjguk added the 3rd party label Nov 11, 2018

@tjguk tjguk referenced this issue Nov 26, 2018

Open

Import issue #652

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment