Skip to content

Add support for wildcard OS name #838

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.

Sign up for GitHub

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

Merged
cottsay merged 2 commits into from
Dec 3, 2021
Merged

Add support for wildcard OS name #838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7594091
Add support for wildcard OS name
cottsay Nov 13, 2021
91cf3a1
Preserve queried os_name and os_version for error messages
cottsay Nov 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/rosdep2/lookup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,17 @@ def get_rule_for_platform(self, os_name, os_version, installer_keys, default_ins
"""
rosdep_key = self.rosdep_key
data = self.data
queried_os = os_name
queried_ver = os_version

if type(data) != dict:
raise InvalidData('rosdep value for [%s] must be a dictionary' % (self.rosdep_key), origin=self.origin)
if os_name not in data:
raise ResolutionError(rosdep_key, data, os_name, os_version, 'No definition of [%s] for OS [%s]' % (rosdep_key, os_name))
if '*' not in data:
raise ResolutionError(rosdep_key, data, queried_os, queried_ver, 'No definition of [%s] for OS [%s]' % (rosdep_key, os_name))
elif type(data['*']) != dict:
raise InvalidData('rosdep value under OS wildcard for [%s] must specify a package manager' % (rosdep_key))
os_name = '*'
data = data[os_name]
return_key = default_installer_key

Expand All @@ -129,10 +135,10 @@ def get_rule_for_platform(self, os_name, os_version, installer_keys, default_ins
# dictionary value.
# if the os_version is not defined and there is no wildcard
if os_version not in data and '*' not in data:
raise ResolutionError(rosdep_key, self.data, os_name, os_version, 'No definition of [%s] for OS version [%s]' % (rosdep_key, os_version))
raise ResolutionError(rosdep_key, self.data, queried_os, queried_ver, 'No definition of [%s] for OS version [%s]' % (rosdep_key, os_version))
# if the os_version has the value None
if os_version in data and data[os_version] is None:
raise ResolutionError(rosdep_key, self.data, os_name, os_version, '[%s] defined as "not available" for OS version [%s]' % (rosdep_key, os_version))
raise ResolutionError(rosdep_key, self.data, queried_os, queried_ver, '[%s] defined as "not available" for OS version [%s]' % (rosdep_key, os_version))
# if os version is not defined (and there is a wildcard) fallback to the wildcard
if os_version not in data:
os_version = '*'
Expand All @@ -146,7 +152,7 @@ def get_rule_for_platform(self, os_name, os_version, installer_keys, default_ins

# Check if the rule is null
if data is None:
raise ResolutionError(rosdep_key, self.data, os_name, os_version, '[%s] defined as "not available" for OS version [%s]' % (rosdep_key, os_version))
raise ResolutionError(rosdep_key, self.data, queried_os, queried_ver, '[%s] defined as "not available" for OS version [%s]' % (rosdep_key, os_version))
if type(data) not in (dict, list, type('str')):
raise InvalidData('rosdep OS definition for [%s:%s] must be a dictionary, string, or list: %s' % (self.rosdep_key, os_name, data), origin=self.origin)

Expand Down
37 changes: 36 additions & 1 deletion test/test_rosdep_lookup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ def test_RosdepDefinition():
except ResolutionError as e:
assert e.rosdep_key == 'trusty_only_key'
assert e.os_name == 'ubuntu'
assert e.os_version == '*'
assert e.os_version == 'lucid'
Copy link
Copy Markdown
Member Author

@cottsay cottsay Nov 16, 2021

Choose a reason for hiding this comment

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

Note that this is a change in the existing behavior, but this pattern doesn't appear anywhere in the rosdep database right now.

Copy link
Copy Markdown
Contributor

@nuclearsandwich nuclearsandwich Nov 16, 2021

Choose a reason for hiding this comment

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

this pattern doesn't appear anywhere in the rosdep database right now.

We don't have anything that fits this exactly but I think any situation where we use the combination of an os_version wildcard with specific os_version: null keys is meant to be covered by this test even if we don't have anywhere where a whole os_name is also null (as we usually just omit the key).

Am I right in thinking that this behavior change is due to 91cf3a1 which preserved the originally requested os_name and os_version?
This definitely seems like an improvement to the error messages for which a case could be made independently from the os_name wildcard support so I see it as an improvement to existing behavior. But I think that the case under test is an

Copy link
Copy Markdown
Member Author

@cottsay cottsay Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Am I right in thinking that this behavior change is due to 91cf3a1 ...?

Correct.

However, if you follow the code, the os_version isn't getting changed until L138 at which point the only possible ResolutionError would be from an explicitly nulled-out rule. Combined, this means that there is only one pattern which will behave differently under 91cf3a1:

foo:
  debian:
    '*': null
    sid: [bar]

This syntax is somewhat redundant, and causes rosdep's resolution error message to change, but not the exception type. When last I surveyed we had no occurrences of '*': null in the database, but it looks like two of them have slipped in. They should probably be removed.

An OS-level null will bypass L138 so the error message won't change. Again, though, we shouldn't have any of these in the database (even though we do).

EDIT: It looks like @clalancette requested the explicit null rules specifically, so I'd guess this is OK now and I missed the memo.
ros/rosdistro#21906 (comment)
ros/rosdistro#29316 (comment)
ros/rosdistro#27390 (comment)

So 91cf3a1 will then result in a change/improvement to the error message for the the latter two PRs.

# tripwire
str(e)
try:
Expand All @@ -202,6 +202,41 @@ def test_RosdepDefinition():
# tripwire
str(e)

definition = RosdepDefinition('invalid_os_wildcard', {'*': ['pytest']}, 'os_wildcard.txt')
try:
val = definition.get_rule_for_platform('debian', 'sid', ['apt', 'source', 'pip'], 'apt')
assert False, 'should have raised: %s' % (str(val))
except InvalidData:
pass

definition = RosdepDefinition('non_debian_key', {'debian': None, '*': {'pip': ['pytest']}}, 'os_wildcard.txt')
try:
val = definition.get_rule_for_platform('debian', 'sid', ['apt', 'source', 'pip'], 'apt')
assert False, 'should have raised: %s' % (str(val))
except ResolutionError as e:
assert e.rosdep_key == 'non_debian_key'
assert e.os_name == 'debian'
assert e.os_version == 'sid'
# tripwire
str(e)

# package manager not supported
try:
val = definition.get_rule_for_platform('ubuntu', 'precise', ['apt', 'source'], 'apt')
assert False, 'should have raised: %s' % (str(val))
except ResolutionError as e:
assert e.rosdep_key == 'non_debian_key'
assert e.os_name == 'ubuntu'
assert e.os_version == 'precise'
# tripwire
str(e)

val = definition.get_rule_for_platform('ubuntu', 'precise', ['apt', 'source', 'pip'], 'apt')
assert val == ('pip', ['pytest']), val

val = definition.get_rule_for_platform('fedora', '35', ['dnf', 'source', 'pip'], 'dnf')
assert val == ('pip', ['pytest']), val

# test reverse merging OS things (first is default)
definition = RosdepDefinition('test', {'debian': 'libtest-dev'}, 'fake-1.txt')
# rule should work as expected before reverse-merge
Expand Down