Skip to content

bpo-30374: Fixed several bugs in win_add2path.py - #1645

Closed
XDcsy wants to merge 3 commits into
python:mainfrom
XDcsy:master
Closed

bpo-30374: Fixed several bugs in win_add2path.py#1645
XDcsy wants to merge 3 commits into
python:mainfrom
XDcsy:master

Conversation

@XDcsy

@XDcsyXDcsy commented May 18, 2017•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
  • Discard DEFAULT = "%PATH%". The system PATH is effective to all users[1] so adding "%PATH%" in user PATH is unnecessary.
  • os.path.isdir() always return False because the input path includes "%APPDATA%" and also because the USER_SITE directory does not yet exist when Python is just installed. It is now deleted to make paths be added correctly.
  • The script compulsively sets the type of PATH var to REG_EXPAND_SZ to make the reference "%APPDATA%" effective[2] . However this is not a good practice (brought forward by Eryk Sun in the issue). Now the script does not change the type of PATH. If it's REG_SZ, the absolute path of %APPDATA% will be added instead.
  • The script needs users to log off and then log back on to take effect. Now it takes effect immediately by broadcasting a WM_SETTINGCHANG message[3] after the change of env vars.
  • Now displays error massages when fails to load PATH values or fails to refresh env vars.

https://bugs.python.org/issue30374

@XDcsy
fix several bugs
ddaf7aa
* Discard the DEFAULT "%PATH%" value.
* Delete wrong `os.path.isdir()`.
* Add absolute path instead of `%APPDATA%...` if the type of PATH is `REG_SZ`.
* Broadcast a `WM_SETTINGCHANG` message after the change of env vars.
* Add codes to deal with possible errors.
@mention-bot

mention-bot commented May 18, 2017

Copy link
Copy Markdown

@neEverett, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @tiran and @birkenfeld to be potential reviewers.

@serhiy-storchaka
serhiy-storchaka requested a review from a teamDecember 8, 2018 10:31
@serhiy-storchakaserhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Dec 8, 2018
zooba
zooba reviewed Feb 2, 2019
View reviewed changes

@zoobazooba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this! Overall they are good looking changes, just a few loose ends to tidy up.

Comment threadTools/scripts/win_add2path.py Outdated
import site
import os
import winreg
import ctypes

@zoobazoobaFeb 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Put in alphabetical order

Comment threadTools/scripts/win_add2path.py Outdated
envpath, dtype = winreg.QueryValueEx(key, PATH)
except FileNotFoundError:
envpath, dtype = "", winreg.REG_EXPAND_SZ
pass

@zoobazoobaFeb 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unnecessary pass

Comment threadTools/scripts/win_add2path.py Outdated
except:
raise OSError("Failed to load PATH value")

if hasattr(site, "USER_SITE") and dtype == winreg.REG_EXPAND_SZ:

@zoobazoobaFeb 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This hasattr check doesn't apply to the subsequent elif block, or rather, if the first check here fails then we may end up crashing in the elif block.

@XDcsy
Tie up some loose ends
0cdb0e8
Fixed the problems mentioned here: python#1645
@XDcsy
Tie up some loose ends
80e8874
Fixed the problems mentioned here: python#1645
@csabella

csabella commented Jan 27, 2020

Copy link
Copy Markdown
Contributor

Closing and reopening to retrigger tests.

@csabellacsabella reopened this Jan 27, 2020
@csabella
csabella requested review from zooba and zwareJanuary 27, 2020 00:01
@github-actions

github-actionsBot commented Feb 19, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Feb 19, 2022
@github-actionsgithub-actionsBot removed the stale Stale PR or inactive for long period of time. label Jul 28, 2022
@github-actions

github-actionsBot commented Aug 28, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Aug 28, 2022
@neEverettneEverettmannequin mentioned this pull request Nov 30, 2022
Closed
@kumaraditya303

kumaraditya303 commented Nov 30, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Closing as win_add2path.py was removed in #98167. thanks

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

Reviewers

@zoobazoobaAwaiting requested review from zooba
@zwarezwareAwaiting requested review from zware

Assignees

No one assigned

Labels

awaiting reviewstaleStale PR or inactive for long period of time.type-bugAn unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@XDcsy@mention-bot@csabella@kumaraditya303@zooba@brettcannon@serhiy-storchaka@the-knights-who-say-ni@ezio-melotti