[#3336] do not add not-found tool paths #3337
Conversation
|
The one Appveyor fail is from a file close issue for which a patch exists elsewhere, not sure why it's intermittent. |
Remove now unneeded code to save/restore the path, since the routine now does not modify the path. Signed-off-by: Mats Wichmann <[email protected]>
When tool modules initialize, they check paths to decide if the underlying tool is actually present. The current checker adds any "default paths" the caller may have supplied (locations like mingw, cygwin, chocolatey install locations, etc.); if there is match from this list, any previous default paths are also kept. To avoid keeping these non-matching paths, restore the original PATH; the caller is responsible for adding to PATH if necessary. Docstring now says so. Note lex and yacc tool modules seem to expect the path-modifying behavior that's being gotten rid of - so they preseve the path first and restore it after. The change here won't break those, but makes the extra behavior unneeded - could remove it. Signed-off-by: Mats Wichmann <[email protected]>
Remove now unneeded code to save/restore the path, since the routine now does not modify the path. Signed-off-by: Mats Wichmann <[email protected]>
src/engine/SCons/Tool/yacc.py
Outdated
| """ | ||
| # save existing path to reset if we don't want to append any paths | ||
| envPath = env['ENV']['PATH'] | ||
| bins = ['bison', 'yacc', 'win_bison'] |
There was a problem hiding this comment.
Can you move this list to the top, name win_bins and have a non-win bins and then use that everywhere instead of having several lists scattered throughout the module?
There was a problem hiding this comment.
no problem, I've got that ready to push... but while I'm at it, the block in yacc.py from lines 145-151 seems like it is redundant now - like the "old version" was still left around when the get_yacc_path function was added. Should this be dropped?
A list of possible executable names are provided in several places in yacc and lex tools, make it a little cleaner by defining once, at the top. Signed-off-by: Mats Wichmann <[email protected]>
|
fwiw, the build fail from appveyor is surpious - something happend during the chocolatey setup, it wasn't from the tests themselves. |
|
So if this test is "wrong", I don't have any other ideas how to test. I don't get how to check a filesystem glob to match jdks without having something in the filesystem to glob against. Maybe just give up on the test? |
|
Unit test's don't touch the filesystem, or shell out and run commands. |
Add a unit test to show find_program_path does not alter env['ENV']['PATH']. A little cleanup in Tool/__init__.py: don't use mutable object as default value in function signature (checkers complain about this); getter/setter usage seemed unnecessary - kept one of the two but use modern syntax (checkers complain about use-before set, which is fixed by the change) Signed-off-by: Mats Wichmann <[email protected]>
| @@ -394,7 +396,8 @@ def _call_env_subst(env, string, *args, **kw): | |||
|
|
|||
|
|
|||
| class _ShLibInfoSupport(object): | |||
| def get_libtype(self): | |||
| @property | |||
| def libtype(self): | |||
There was a problem hiding this comment.
Is this related to the bug?
Signed-off-by: Mats Wichmann <[email protected]>
Signed-off-by: Mats Wichmann <[email protected]>
Resolve Issue #3336
When tool modules initialize, they check paths to decide if the underlying tool is actually present. The current checker adds any "default paths" the caller may have supplied (locations like mingw, cygwin, chocolatey install locations, etc.); if there is match from this list, any previous default paths are also kept. To avoid keeping these non-matching paths, restore the original PATH; the caller is responsible for adding to PATH if necessary. Docstring now says so.
Note lex and yacc tool modules expected the path-modifying behavior that's being gotten rid of - so they preseve the path first and restore it after. That has been rolled back out.
The
swig.pychange is not functional, it just aligns the usage with what is introduced elsewhere.Signed-off-by: Mats Wichmann [email protected]
Contributor Checklist:
master/src/CHANGES.txtdirectory (and read theREADME.txtin that directory)