Skip to content

improve support for null entries#726

Merged
tfoote merged 4 commits intoros-infrastructure:masterfrom
mikaelarguedas:fix_platform_null
Mar 27, 2020
Merged

improve support for null entries#726
tfoote merged 4 commits intoros-infrastructure:masterfrom
mikaelarguedas:fix_platform_null

Conversation

@mikaelarguedas
Copy link
Copy Markdown
Contributor

syntax like 'ubuntu: null' will now be raising ResolutionError with
relevant error message instead of InvalidData

This came up recently with ros/rosdistro#21906
This should also fix / improve error message for issues like #689

Details

before:

root@cd07c77952b3:/# rosdep resolve apparmor --os fedora:30

ERROR: Rosdep experienced an error: rosdep OS definition for [apparmor:fedora] must be a dictionary, string, or list: None
Please go to the rosdep page [1] and file a bug report with the stack trace below.
[1] : http://www.ros.org/wiki/rosdep

rosdep version: 0.17.1

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/rosdep2/main.py", line 144, in rosdep_main
    exit_code = _rosdep_main(args)
  File "/usr/local/lib/python3.6/dist-packages/rosdep2/main.py", line 424, in _rosdep_main
    return _rosdep_args_handler(command, parser, options, args)
  File "/usr/local/lib/python3.6/dist-packages/rosdep2/main.py", line 446, in _rosdep_args_handler
    return command_handlers[command](args, options)
  File "/usr/local/lib/python3.6/dist-packages/rosdep2/main.py", line 865, in command_resolve
    rule_installer, rule = d.get_rule_for_platform(os_name, os_version, installer_keys, default_key)
  File "/usr/local/lib/python3.6/dist-packages/rosdep2/lookup.py", line 151, in get_rule_for_platform
    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)
rosdep2.core.InvalidData: rosdep OS definition for [apparmor:fedora] must be a dictionary, string, or list: None

after:

root@cd07c77952b3:/# rosdep resolve apparmor --os fedora:30

ERROR: [apparmor] defined as "not available" for OS version [30]

[apparmor] defined as "not available" for OS version [30]
	rosdep key : apparmor
	OS name    : fedora
	OS version : 30
	Data: 
debian:
- apparmor
fedora: null
ubuntu:
- apparmor

Second commit improves the formatting, but can be ommitted if disruptive as it;s not really related to this fix

Details

before:

root@cd07c77952b3:/# rosdep resolve apparmor --os fedora:30

ERROR: [apparmor] defined as "not available" for OS version [30]

[apparmor] defined as "not available" for OS version [30]
	rosdep key : apparmor
	OS name    : fedora
	OS version : 30
	Data: debian:
- apparmor
fedora: null
ubuntu:
- apparmor

after:

root@cd07c77952b3:/# rosdep resolve apparmor --os fedora:30

ERROR: [apparmor] defined as "not available" for OS version [30]

[apparmor] defined as "not available" for OS version [30]
	rosdep key : apparmor
	OS name    : fedora
	OS version : 30
	Data: 
debian:
- apparmor
fedora: null
ubuntu:
- apparmor

Signed-off-by: Mikael Arguedas mikael.arguedas@gmail.com

@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 23, 2019

Codecov Report

Merging #726 into master will increase coverage by 0.48%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #726      +/-   ##
==========================================
+ Coverage   75.85%   76.34%   +0.48%     
==========================================
  Files          40       40              
  Lines        3148     3293     +145     
==========================================
+ Hits         2388     2514     +126     
- Misses        760      779      +19
Impacted Files Coverage Δ
src/rosdep2/lookup.py 87.82% <100%> (+0.09%) ⬆️
src/rosdep2/platforms/source.py 90.5% <0%> (-0.06%) ⬇️
src/rosdep2/_version.py 100% <0%> (ø) ⬆️
src/rosdep2/meta.py 100% <0%> (ø) ⬆️
src/rosdep2/platforms/alpine.py 100% <0%> (ø) ⬆️
src/rosdep2/sources_list.py 87.74% <0%> (+1.09%) ⬆️
src/rosdep2/main.py 54.49% <0%> (+5.65%) ⬆️

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 73dd991...527de9f. Read the comment docs.

@mikaelarguedas mikaelarguedas force-pushed the fix_platform_null branch 2 times, most recently from e51d98f to ba1b2a1 Compare October 23, 2019 13:37
Copy link
Copy Markdown
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

This is great. It's come up several times now, and as python 2 packages drop like flies out of Fedora and EPEL, it's only going to get worse.

Thanks!

@nuclearsandwich nuclearsandwich self-requested a review November 5, 2019 18:02
@mikaelarguedas
Copy link
Copy Markdown
Contributor Author

@cottsay @nuclearsandwich friendly 🛎️

@mikaelarguedas
Copy link
Copy Markdown
Contributor Author

@cottsay @nuclearsandwich is there anything holding this approved PR ?

Copy link
Copy Markdown
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.

I think this is much clearer. Love that this has tests. Thanks @mikaelarguedas

@nuclearsandwich
Copy link
Copy Markdown
Contributor

is there anything holding this approved PR ?

Not from my end. Thanks for the re-ping. I've kicked CI up again and if it comes back 🆗 I'll merge. I think @cottsay has some additional PRs that need review before the next release is made so this may sit on master for a while longer but I'll use this as an impelling force to get those reviewed, merged, and released as well.

@mikaelarguedas
Copy link
Copy Markdown
Contributor Author

Sounds good, CI will most likely need #739 to come back ✔️

Copy link
Copy Markdown
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

This looks good but needs a rebase to pass CI

mikaelarguedas and others added 4 commits March 26, 2020 11:41
syntax like 'ubuntu: null' will now be raising ResolutionError with
relevant error message instead of InvalidData with obscure error message
will also cover syntax like:
"
ubuntu:
  '*': null
  bionic: [foobar]
"

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Co-Authored-By: Scott K Logan <logans@cottsay.net>
@mikaelarguedas
Copy link
Copy Markdown
Contributor Author

rebased and passing CI

@tfoote tfoote merged commit 50ee23e into ros-infrastructure:master Mar 27, 2020
@mikaelarguedas mikaelarguedas deleted the fix_platform_null branch March 27, 2020 06:57
gstavrinos pushed a commit to gstavrinos/rosdep that referenced this pull request Nov 12, 2020
* Add Suite3 option with Ubuntu Focal (ros-infrastructure#734)

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

* [update] Allow to process single ROS distro, fix 723 (ros-infrastructure#738)

* [update] skip other distro if --rosdistro passed

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

* ignore argument if specified distro doesnt exist

* address review comments

* update help message for rosdistro

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

* Fix CI and reduce CI time (ros-infrastructure#739)

* test newer python
* use yaml.safe_load
* pin PyYAML version for Python 3.4
* pass user flag to pip when needed
* move slow jobs to the top to reduce CI time

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

* Strip Alpine's patch version from OS codename (ros-infrastructure#716)

* Resolve Alpine os_version_type using OsDetect
* Reduced Alpine OS VERSION to Major.Minor

* improve support for null entries (ros-infrastructure#726)

* support null for entire OS and not only OS version

syntax like 'ubuntu: null' will now be raising ResolutionError with
relevant error message instead of InvalidData with obscure error message
will also cover syntax like:
"
ubuntu:
  '*': null
  bionic: [foobar]
"

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

Co-authored-by: Scott K Logan <logans@cottsay.net>

* Use DNF installer on RHEL 8 and newer (ros-infrastructure#713)

RHEL/CentOS 8 uses DNF by default.

* Updates to YUM and DNF (ros-infrastructure#640)

Output YUM, DNF and RPM versions with `--all-versions` and fix the format guide's default PM for Fedora.

* tests: don't assume euid != 0 (ros-infrastructure#703)

If tests rely on the environment being a certain way, they should ensure
it meets its expectations. Update the tests to ensure they're testing
multiple euid conditions, and setting it explicitly where required.

Fix ros-infrastructure#702

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

* openSUSE package query  and install enhancements (ros-infrastructure#729)

* Enable PIP installer for openSUSE

* openSUSE package detection with RPM capabilities

Packages sometimes get renamed and their old name is kept as an rpm capability (like an alias), so the additional flag `--whatprovides` is passed to the `rpm` query.

* Fix conditional dependencies when one package uses manifest.xml (ros-infrastructure#737)

* Fix conditional dependencies when one package uses manifest.xml

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Return rosdeps as a list to avoid breaking the interface.

Co-authored-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Guard next(inter) (ros-infrastructure#701)

* guard next(inter)

  Fix ros-infrastructure#691

Signed-off-by: artivis <jeremie.deray@canonical.com>

* Handle StopIteration with slightly less line noise.

Co-authored-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* [Windows] Add console script entry point (ros-infrastructure#656)

* Add console script entry point

Add console script entry point for platforms (e.g. Windows) not supporting shebang.

* remove scripts entry since we are using console_scripts.

* fix bad merge.

* fix SKIP_PYTHON_SCRIPTS case.

Co-authored-by: Lou Amadio <ooeygui@users.noreply.github.com>

* Depend on modules packages only to allow co-installability. (ros-infrastructure#750)

When the rosdep modules package was split in [ros-infrastructure#731] the module
dependencies weren't updated to only depend on modules packages
themselves which prevents rosdep modules for python2 and python3 from
actually being co-installable.

I haven't yet audited to make sure there's no cli usage of these tools
in the modules but it's Friday afternoon and I wanted to get this inked
before I walked away.

When I return to it I'll check for that before proceeding for reviews.

* 0.19.0

* also install buildtool_export_depends (ros-infrastructure#753)

not adding 'exec_depends' or 'build_export_depends' are both are included inside 'run_depends'

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

* Drop shebang from a non-executable file (ros-infrastructure#755)

* add alias for Pop! OS (ros-infrastructure#757)

* add alias for Pop! OS

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

Co-authored-by: Shane Loretz <sloretz@openrobotics.org>
Co-authored-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Co-authored-by: Mark Hedley Jones <MarkHedleyJones@gmail.com>
Co-authored-by: Scott K Logan <logans@cottsay.net>
Co-authored-by: Kyle Fazzari <kyle@canonical.com>
Co-authored-by: Bjar Ne <43565432+gleichdick@users.noreply.github.com>
Co-authored-by: Jeremie Deray <deray.jeremie@gmail.com>
Co-authored-by: Sean Yen <seanyen@microsoft.com>
Co-authored-by: Lou Amadio <ooeygui@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants