Skip to content

Commit

Permalink
Merge pull request #4590 from mwichmann/issue/unique-delete-existing
Browse files Browse the repository at this point in the history 
Fix bug with unique adds and delete_existing
  • Loading branch information
@bdbaddog
bdbaddog authored Aug 29, 2024
2 parents 46cadec + f67bfba commit 53d29af
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 52 deletions.
2 changes: 2 additions & 0 deletions CHANGES.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER
- Improve wording of manpage "Functions and Environment Methods" section.
Make doc function signature style more consistent - tweaks to AddOption,
DefaultEnvironment and Tool,.
- Fix a problem with AppendUnique and PrependUnique where a value could
be erroneously removed due to a substring match.
- Fix handling of ListVariable when supplying a quoted choice containing
a space character (issue #4585).

Expand Down
2 changes: 2 additions & 0 deletions RELEASE.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ FIXES
to be avaiable. `BoolVariable`, `EnumVariable`, `ListVariable`,
`PackageVariable` and `PathVariable` are added to `__all__`,
so this form of import should now work again.
- Fix a problem with AppendUnique and PrependUnique where a value could
be erroneously removed due to a substring match.
- Fix handling of ListVariable when supplying a quoted choice containing
a space character (issue #4585).

Expand Down
42 changes: 26 additions & 16 deletions SCons/Environment.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1512,11 +1512,17 @@ def AppendENVPath(self, name, newpath, envname: str='ENV',

self._dict[envname][name] = nv

def AppendUnique(self, delete_existing: bool=False, **kw) -> None:
"""Append values to existing construction variables
in an Environment, if they're not already there.
If delete_existing is True, removes existing values first, so
values move to end.
def AppendUnique(self, delete_existing: bool = False, **kw) -> None:
"""Append values uniquely to existing construction variables.
Similar to :meth:`Append`, but the result may not contain duplicates
of any values passed for each given key (construction variable),
so an existing list may need to be pruned first, however it may still
contain other duplicates.
If *delete_existing* is true, removes existing values first, so values
move to the end; otherwise (the default) values are skipped if
already present.
"""
kw = copy_non_reserved_keywords(kw)
for key, val in kw.items():
Expand All @@ -1539,12 +1545,11 @@ def AppendUnique(self, delete_existing: bool=False, **kw) -> None:
val = [x for x in val if x not in dk]
self._dict[key] = dk + val
else:
# val is not a list, so presumably a scalar (likely str).
dk = self._dict[key]
if is_List(dk):
# By elimination, val is not a list. Since dk is a
# list, wrap val in a list first.
if delete_existing:
dk = list(filter(lambda x, val=val: x not in val, dk))
dk = [x for x in dk if x != val]
self._dict[key] = dk + [val]
else:
if val not in dk:
Expand Down Expand Up @@ -1939,11 +1944,17 @@ def PrependENVPath(self, name, newpath, envname: str='ENV',

self._dict[envname][name] = nv

def PrependUnique(self, delete_existing: bool=False, **kw) -> None:
"""Prepend values to existing construction variables
in an Environment, if they're not already there.
If delete_existing is True, removes existing values first, so
values move to front.
def PrependUnique(self, delete_existing: bool = False, **kw) -> None:
"""Prepend values uniquely to existing construction variables.
Similar to :meth:`Prepend`, but the result may not contain duplicates
of any values passed for each given key (construction variable),
so an existing list may need to be pruned first, however it may still
contain other duplicates.
If *delete_existing* is true, removes existing values first, so values
move to the front; otherwise (the default) values are skipped if
already present.
"""
kw = copy_non_reserved_keywords(kw)
for key, val in kw.items():
Expand All @@ -1966,12 +1977,11 @@ def PrependUnique(self, delete_existing: bool=False, **kw) -> None:
val = [x for x in val if x not in dk]
self._dict[key] = val + dk
else:
# val is not a list, so presumably a scalar (likely str).
dk = self._dict[key]
if is_List(dk):
# By elimination, val is not a list. Since dk is a
# list, wrap val in a list first.
if delete_existing:
dk = [x for x in dk if x not in val]
dk = [x for x in dk if x != val]
self._dict[key] = [val] + dk
else:
if val not in dk:
Expand Down
93 changes: 57 additions & 36 deletions SCons/EnvironmentTests.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1193,18 +1193,6 @@ def test_ENV(self) -> None:

def test_ReservedVariables(self) -> None:
"""Test warning generation when reserved variable names are set"""

reserved_variables = [
'CHANGED_SOURCES',
'CHANGED_TARGETS',
'SOURCE',
'SOURCES',
'TARGET',
'TARGETS',
'UNCHANGED_SOURCES',
'UNCHANGED_TARGETS',
]

warning = SCons.Warnings.ReservedVariableWarning
SCons.Warnings.enableWarningClass(warning)
old = SCons.Warnings.warningAsException(1)
Expand Down Expand Up @@ -1759,19 +1747,26 @@ def test_AppendENVPath(self) -> None:
ENV={'PATH': r'C:\dir\num\one;C:\dir\num\two'},
MYENV={'MYPATH': r'C:\mydir\num\one;C:\mydir\num\two'},
)

# have to include the pathsep here so that the test will work on UNIX too.
env1.AppendENVPath('PATH', r'C:\dir\num\two', sep=';')
env1.AppendENVPath('PATH', r'C:\dir\num\three', sep=';')
env1.AppendENVPath('MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';')
assert (
env1['ENV']['PATH'] == r'C:\dir\num\one;C:\dir\num\two;C:\dir\num\three'
), env1['ENV']['PATH']

# add nonexisting - at end
env1.AppendENVPath('MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';')
assert (
env1['MYENV']['MYPATH'] == r'C:\mydir\num\one;C:\mydir\num\two;C:\mydir\num\three'
), env1['MYENV']['MYPATH']

# add existing with delete_existing true - moves to the end
env1.AppendENVPath(
'MYPATH', r'C:\mydir\num\one', 'MYENV', sep=';', delete_existing=1
'MYPATH', r'C:\mydir\num\one', 'MYENV', sep=';', delete_existing=True
)
# this should do nothing since delete_existing is 0
# this should do nothing since delete_existing is false (the default)
env1.AppendENVPath('MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';')
assert (
env1['MYENV']['MYPATH'] == r'C:\mydir\num\two;C:\mydir\num\three;C:\mydir\num\one'
), env1['MYENV']['MYPATH']
Expand All @@ -1783,6 +1778,7 @@ def test_AppendENVPath(self) -> None:
env1.AppendENVPath('PATH', env1.fs.Dir('sub2'), sep=';')
assert env1['ENV']['PATH'] == p + ';sub1;sub2', env1['ENV']['PATH']


def test_AppendUnique(self) -> None:
"""Test appending to unique values to construction variables
Expand Down Expand Up @@ -1832,34 +1828,46 @@ def test_AppendUnique(self) -> None:
assert env['CCC1'] == 'c1', env['CCC1']
assert env['CCC2'] == ['c2'], env['CCC2']
assert env['DDD1'] == ['a', 'b', 'c'], env['DDD1']
assert env['LL1'] == [env.Literal('a literal'), env.Literal('b literal')], env['LL1']
assert env['LL2'] == [env.Literal('c literal'), env.Literal('b literal'), env.Literal('a literal')], [str(x) for x in env['LL2']]
assert env['LL1'] == [env.Literal('a literal'), \
env.Literal('b literal')], env['LL1']
assert env['LL2'] == [
env.Literal('c literal'),
env.Literal('b literal'),
env.Literal('a literal'),
], [str(x) for x in env['LL2']]

env.AppendUnique(DDD1='b', delete_existing=True)
assert env['DDD1'] == ['a', 'c', 'b'], env['DDD1'] # b moves to end

env.AppendUnique(DDD1 = 'b', delete_existing=1)
assert env['DDD1'] == ['a', 'c', 'b'], env['DDD1'] # b moves to end
env.AppendUnique(DDD1 = ['a','b'], delete_existing=1)
assert env['DDD1'] == ['c', 'a', 'b'], env['DDD1'] # a & b move to end
env.AppendUnique(DDD1 = ['e','f', 'e'], delete_existing=1)
assert env['DDD1'] == ['c', 'a', 'b', 'f', 'e'], env['DDD1'] # add last
env.AppendUnique(DDD1=['a', 'b'], delete_existing=True)
assert env['DDD1'] == ['c', 'a', 'b'], env['DDD1'] # a & b move to end

env.AppendUnique(DDD1=['e', 'f', 'e'], delete_existing=True)
assert env['DDD1'] == ['c', 'a', 'b', 'f', 'e'], env['DDD1'] # add last

# issue regression: substrings should not be deleted
env.AppendUnique(BBB4='b4.newer', delete_existing=True)
assert env['BBB4'] == ['b4', 'b4.new', 'b4.newer'], env['BBB4']

env['CLVar'] = CLVar([])
env.AppendUnique(CLVar = 'bar')
env.AppendUnique(CLVar='bar')
result = env['CLVar']
assert isinstance(result, CLVar), repr(result)
assert result == ['bar'], result

env['CLVar'] = CLVar(['abc'])
env.AppendUnique(CLVar = 'bar')
env.AppendUnique(CLVar='bar')
result = env['CLVar']
assert isinstance(result, CLVar), repr(result)
assert result == ['abc', 'bar'], result

env['CLVar'] = CLVar(['bar'])
env.AppendUnique(CLVar = 'bar')
env.AppendUnique(CLVar='bar')
result = env['CLVar']
assert isinstance(result, CLVar), repr(result)
assert result == ['bar'], result


def test_Clone(self) -> None:
"""Test construction environment cloning.
Expand Down Expand Up @@ -2501,18 +2509,26 @@ def test_PrependENVPath(self) -> None:
ENV={'PATH': r'C:\dir\num\one;C:\dir\num\two'},
MYENV={'MYPATH': r'C:\mydir\num\one;C:\mydir\num\two'},
)

# have to include the pathsep here so that the test will work on UNIX too.
env1.PrependENVPath('PATH', r'C:\dir\num\two', sep=';')
env1.PrependENVPath('PATH', r'C:\dir\num\three', sep=';')
assert (
env1['ENV']['PATH'] == r'C:\dir\num\three;C:\dir\num\two;C:\dir\num\one'
), env1['ENV']['PATH']


# add nonexisting - at front
env1.PrependENVPath('MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';')
assert (
env1['MYENV']['MYPATH'] == r'C:\mydir\num\three;C:\mydir\num\one;C:\mydir\num\two'
), env1['MYENV']['MYPATH']

# add existing - moves to the front
env1.PrependENVPath('MYPATH', r'C:\mydir\num\one', 'MYENV', sep=';')
# this should do nothing since delete_existing is 0
# this should do nothing since delete_existing is false
env1.PrependENVPath(
'MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';', delete_existing=0
'MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';', delete_existing=False
)
assert (
env1['MYENV']['MYPATH'] == r'C:\mydir\num\one;C:\mydir\num\three;C:\mydir\num\two'
Expand All @@ -2525,6 +2541,7 @@ def test_PrependENVPath(self) -> None:
env1.PrependENVPath('PATH', env1.fs.Dir('sub2'), sep=';')
assert env1['ENV']['PATH'] == 'sub2;sub1;' + p, env1['ENV']['PATH']


def test_PrependUnique(self) -> None:
"""Test prepending unique values to construction variables
Expand Down Expand Up @@ -2570,32 +2587,36 @@ def test_PrependUnique(self) -> None:
assert env['CCC2'] == ['c2'], env['CCC2']
assert env['DDD1'] == ['a', 'b', 'c'], env['DDD1']

env.PrependUnique(DDD1 = 'b', delete_existing=1)
assert env['DDD1'] == ['b', 'a', 'c'], env['DDD1'] # b moves to front
env.PrependUnique(DDD1 = ['a','c'], delete_existing=1)
assert env['DDD1'] == ['a', 'c', 'b'], env['DDD1'] # a & c move to front
env.PrependUnique(DDD1 = ['d','e','d'], delete_existing=1)
env.PrependUnique(DDD1='b', delete_existing=True)
assert env['DDD1'] == ['b', 'a', 'c'], env['DDD1'] # b moves to front
env.PrependUnique(DDD1=['a', 'c'], delete_existing=True)
assert env['DDD1'] == ['a', 'c', 'b'], env['DDD1'] # a & c move to front
env.PrependUnique(DDD1=['d', 'e', 'd'], delete_existing=True)
assert env['DDD1'] == ['d', 'e', 'a', 'c', 'b'], env['DDD1']

# issue regression: substrings should not be deleted
env.PrependUnique(BBB4='b4.newer', delete_existing=True)
assert env['BBB4'] == ['b4.newer', 'b4.new', 'b4'], env['BBB4']

env['CLVar'] = CLVar([])
env.PrependUnique(CLVar = 'bar')
env.PrependUnique(CLVar='bar')
result = env['CLVar']
assert isinstance(result, CLVar), repr(result)
assert result == ['bar'], result

env['CLVar'] = CLVar(['abc'])
env.PrependUnique(CLVar = 'bar')
env.PrependUnique(CLVar='bar')
result = env['CLVar']
assert isinstance(result, CLVar), repr(result)
assert result == ['bar', 'abc'], result

env['CLVar'] = CLVar(['bar'])
env.PrependUnique(CLVar = 'bar')
env.PrependUnique(CLVar='bar')
result = env['CLVar']
assert isinstance(result, CLVar), repr(result)
assert result == ['bar'], result


def test_Replace(self) -> None:
"""Test replacing construction variables in an Environment
Expand Down

0 comments on commit 53d29af

Please sign in to comment.