Skip to content
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.

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

Add support for wildcard OS name #838

Merged
merged 2 commits into from
Dec 3, 2021
Merged

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Nov 13, 2021

This is an implementation of my proposed changes to REP 111.

The motivations and behaviors behind this implementation are discussed there.

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2021

Codecov Report

Merging #838 (7594091) into master (be780c4) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #838      +/-   ##
==========================================
+ Coverage   74.96%   74.99%   +0.03%     
==========================================
  Files          42       42              
  Lines        3311     3315       +4     
==========================================
+ Hits         2482     2486       +4     
  Misses        829      829              
Impacted Files Coverage Δ
src/rosdep2/lookup.py 88.08% <100.00%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be780c4...7594091. Read the comment docs.

@cottsay cottsay requested a review from tfoote November 16, 2021 22:22
@@ -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
Member Author

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
Contributor

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
Member Author

@cottsay cottsay Nov 17, 2021

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.

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

Looks like a solid improvement!

One thing I did notice while reviewing which isn't a blocker for this PR is that the definition test is growing towards 160 lines of asserts all with varying state. It would be nice to break that up into smaller test methods at some point.

@@ -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
Contributor

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

@cottsay cottsay merged commit 5ebeb46 into master Dec 3, 2021
@cottsay cottsay deleted the cottsay/rep111-os-wildcards branch December 3, 2021 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants