Fix the fact that spyder_win_post_install.py can't be run from pip #2377

mikofski · 2015-04-28T21:25:22Z

add missing bdist_wininst functions from pywin32_postinstall.py:
file_created(), directory_created(), get_root_key(), create_shortcut()
and get_special_folder_path()
test for pythoncom and return message "pywin32 required" on ImportError
replace CSIDL_COMMON_PROGRAMS with CSIDL_PROGRAMS to be consistent with
desktop shortcut which is in user profile, and so that user doesn't have
to elevate rights in order to install Spyder
add TODO to remove error message "try running installer as
administrator" since it's unnecessary if admin rights not required.
if running spyder_win_post_install.py -install manually after installing
with pip, then spyder.ico and spyder_light.ico are in the
Python27\Scripts\ folder not in the distribution folder, so check if
ico_dir exists and use absolute path to spyder_win_post_install.__file__
clean up desktop and start menu shortcuts on removal
provide feedback on successful removal of context, desktop and start
menu shortcuts

Signed-off-by: Mark Mikofski mark.mikofski@sunpowercorp.com

* add missing bdist_wininst functions from pywin32_postinstall.py: file_created(), directory_created(), get_root_key(), create_shortcut() and get_special_folder_path() * test for pythoncom and return message "pywin32 required" on ImportError * replace CSIDL_COMMON_PROGRAMS with CSIDL_PROGRAMS to be consistent with desktop shortcut which is in user profile, and so that user doesn't have to elevate rights in order to install spyder * add TODO to remove error message "try running installer as administrator" since it's unnecessary if admin rights not required. * if running spyder_win_post_install.py -install manually after installing with pip, then spyder.ico and spyder_light.ico are in the Python27\Scripts\ folder not in the distribution folder, so check if ico_dir exists and use absolute path to spyder_win_post_install.__file__ * clean up desktop and start menu shortcuts on removal * provide feedback on successful removal of context, desktop and start menu shortcuts Signed-off-by: Mark Mikofski <mark.mikofski@sunpowercorp.com>

ccordoba12 · 2015-04-30T22:38:41Z

scripts/spyder_win_post_install.py

@@ -21,17 +21,95 @@
 KEY_C1 = KEY_C0 + r"\command"


+# ability to run spyder-win-post-install outside of bdist_wininst installer
+# copied from pywin32-win-post-install


Could you post here a link to Github (or Bitbucket, etc) where we can read pywin32-win-post-install code?

The pywin32 mercurial repository is on sourceforge. The most recent build is tagged as b219. Here's a link to pywin32_postinstall.py.

Sorry for not being clear enough :-) Please add the link to the file, below the last line of your comment.

check out 330b0aa

ccordoba12 · 2015-05-01T22:23:43Z

One thing that I'd like to know is if it's possible to tell pip to run this script after install/uninstall steps.

I mean, can't we add something to setup.py to make this happen automatically?

mikofski · 2015-05-02T00:04:15Z

That would be very nice if the post install script could be run automatically, although I was thinking of making that a separate feature after this one gets pulled.

Another feature I thought would be nice is a post install script for other platforms that does the equivalent for linux and mac osx. IE on linux it could create a desktop file in .local/share/applications/ called spyder.desktop that would add spyder to the launcher. And in mac osx it could drop an .app file in the applications drawer that launched spyder instead of having to launch it from the terminal.

Back to your question, though, I don't know how to get pip to run the script. I don't if you can add it to setup.py but it seems plausible. It's a good place to start. But I'm okay with just running it manually. When I set up Python for other engineers here at SunPower, I usually use pip, and then manually make a shortcut to spyder on the desktop that runs C:\Python27\python.exe C:\Python27\scripts\spyder and then manually change the icon to C:\Python27\scripts\spyder.ico - so having this post installer will greatly improve the setup procedure since it will also add the Windows Explorer Context menu support.

Thanks!

* add link to `pwin32_postinstall.py` in hg repo on sf.net * add comments to explain `file_created()` and `directory_created()` dummy functions, and how to remove shortcuts and startmenu directory using `spyder_win_post_install.py` with `-remove` option Signed-off-by: Mark Mikofski <mark.mikofski@sunpowercorp.com>

ccordoba12 · 2015-06-20T20:52:59Z

scripts/spyder_win_post_install.py

+    def get_root_hkey():
+        try:
+            winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE,
+                           root_key_name, 0, winreg.KEY_CREATE_SUB_KEY)


root_key_name is not defined anywhere in your PR, so this call is going to fail, right?

@ccordoba12 good catch, on windows this key is hkey_local_machine\Software\Python\PythonCore\2.7 for python installed with msi, however, I don't know what it would be on a windows system that has a bootstrapped Python, eg: WinPython, portable python or any python just unzipped in and executed from a standalone folder.

This is only run by bdist_wininst and it's sole purpose is to determine if the user has elevated their rights, if so then use hkey_local_machine and if not then use hkey_current_user. Therefore even if this call fails, it will just fall back to hkey_current_user which is what we want anyway, so it doesn't matter. But for those that have installed official python, and have admin rights, then it will use the system registry keys.

Ok, then we can leave it without problems :-)

Please address my other comments :-)

ccordoba12 · 2015-06-20T21:05:35Z

Ok, my final review is done. Sorry for taking so much time to do it :-)

Please try to address my comments later today or tomorrow morning, if you want to include this PR in 2.3.5. Else, I'll leave it for 3.0.

Signed-off-by: Mark Mikofski <mark.mikofski@sunpowercorp.com>

* startmenu items are added to users startmenu not system, so running as admin will not help if install fails

Signed-off-by: Mark Mikofski <mark.mikofski@sunpowercorp.com>

mikofski · 2015-06-20T22:45:58Z

Thanks @ccordoba12 for your review, I have addressed your current comments:

define root_key_name
complete TODO to remove "run -install with admin rights"
only run manual -remove if is_bdist_wininst == False

Nodd · 2015-06-20T23:05:24Z

scripts/spyder_win_post_install.py

+            CSIDL_LOCAL_APPDATA CSIDL_APPDATA CSIDL_COMMON_DESKTOPDIRECTORY
+            CSIDL_DESKTOPDIRECTORY CSIDL_COMMON_STARTUP CSIDL_STARTUP
+            CSIDL_COMMON_PROGRAMS CSIDL_PROGRAMS CSIDL_PROGRAM_FILES_COMMON
+            CSIDL_PROGRAM_FILES CSIDL_FONTS""".split():


Why not creating a list directly instead of splitting a string ?
Performance-wise it won't matter but it would be more obvious to understand. I had to read these lines 3 times to spot the .split() here.

+1 to this last comment from @Nodd.

I'll do this myself after merging.

ccordoba12 · 2015-06-20T23:33:11Z

@mikofski, thanks for the quick response! I'll merge it tomorrow and backport it to 2.3.5 (which will be released tomorrow too :-)

Fix the fact that spyder_win_post_install.py can't be run from pip

…bdist_wininst

ccordoba12 · 2015-06-22T16:22:17Z

@mikofski, I tested your work before releasing 2.3.5 and it's working great! Thanks a lot for it.

mikofski · 2015-06-22T16:59:08Z

That's great news! Glad I could contribute.

mikofski force-pushed the master branch 2 times, most recently from 73fa06c to 1ad9e63 Compare April 28, 2015 22:18

ccordoba12 changed the title ~~fixes #2188 can't run win_post_install from pip~~ Fix the fact that spyder_win_post_install.py can't be run from pip Apr 30, 2015

ccordoba12 added type:Techdebt type:Enhancement labels Apr 30, 2015

ccordoba12 added this to the v3.0 milestone Apr 30, 2015

ccordoba12 self-assigned this Apr 30, 2015

ccordoba12 reviewed Apr 30, 2015
View reviewed changes

ccordoba12 modified the milestones: v2.3.5, v3.0 Jun 15, 2015

ccordoba12 reviewed Jun 20, 2015
View reviewed changes

mikofski added 3 commits June 20, 2015 14:47

define root_key_name used by get_root_hkey()

572e593

Signed-off-by: Mark Mikofski <mark.mikofski@sunpowercorp.com>

complete TODO: remove message "try running -install as admin"

2f058b2

* startmenu items are added to users startmenu not system, so running as admin will not help if install fails

only run manual -remove if is_bdist_wininst == False

47537bf

Signed-off-by: Mark Mikofski <mark.mikofski@sunpowercorp.com>

Nodd reviewed Jun 20, 2015
View reviewed changes

ccordoba12 added a commit that referenced this pull request Jun 21, 2015

Merge pull request #2377 from mikofski/master

0276606

Fix the fact that spyder_win_post_install.py can't be run from pip

ccordoba12 merged commit 0276606 into spyder-ide:master Jun 21, 2015

ccordoba12 added a commit that referenced this pull request Jun 22, 2015

Minor fixes after PR #2377

ee2a44b

ccordoba12 added a commit that referenced this pull request Jun 22, 2015

Backport PR #2377: Make spyder_win_post_install.py to run outside of …

89ea923

…bdist_wininst

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the fact that spyder_win_post_install.py can't be run from pip #2377

Fix the fact that spyder_win_post_install.py can't be run from pip #2377

mikofski commented Apr 28, 2015

ccordoba12 Apr 30, 2015

mikofski Apr 30, 2015

ccordoba12 May 1, 2015

mikofski May 2, 2015

ccordoba12 commented May 1, 2015

mikofski commented May 2, 2015

ccordoba12 Jun 20, 2015

mikofski Jun 20, 2015

ccordoba12 Jun 20, 2015

ccordoba12 commented Jun 20, 2015

mikofski commented Jun 20, 2015

Nodd Jun 20, 2015

ccordoba12 Jun 20, 2015

ccordoba12 Jun 21, 2015

ccordoba12 commented Jun 20, 2015

ccordoba12 commented Jun 22, 2015

mikofski commented Jun 22, 2015

Fix the fact that spyder_win_post_install.py can't be run from pip #2377

Fix the fact that spyder_win_post_install.py can't be run from pip #2377

Conversation

mikofski commented Apr 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented May 1, 2015

mikofski commented May 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Jun 20, 2015

mikofski commented Jun 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Jun 20, 2015

ccordoba12 commented Jun 22, 2015

mikofski commented Jun 22, 2015