Selection of dependency types with cmd line option by nalt · Pull Request #727 · ros-infrastructure/rosdep

nalt · 2019-10-28T13:18:20Z

Select dependency types (build, buildtool, run, test) to install with --dependency-types. can be given multiple. Standard behaviour (all types) unchanged.

In rospack.py, a fake and empty options object was created for the call to _get_default_RosdepLookup. This was replaced by a similarly hacky global object for command line options.

nuclearsandwich · 2020-04-02T00:24:13Z

Question for other rosdep contributors and maintainer: An approach similar to this one was proposed in #646 (comment) with a slightly different interface. Does anyone have a preference between the two and if so for which one and why?

gavanderhoorn · 2020-04-06T12:15:02Z

@nuclearsandwich: you are essentially asking whether ppl would have a preference for:

--dependency-types <names_of_dep_types_separated_by_spaces>

or:

--dependency-types <name_of_dep_type> --dependency-types <name_of_dep_type> etc

right?

NikolausDemmel · 2020-04-06T13:24:28Z

@nuclearsandwich: you are essentially asking whether ppl would have a preference for:
--dependency-types <names_of_dep_types_separated_by_spaces>
or:
--dependency-types <name_of_dep_type> --dependency-types <name_of_dep_type> etc
right?

Side remark: On this question there is precedent for offering both options at the same time in rosdep:

rosdep/src/rosdep2/main.py

Lines 404 to 406 in c1c220e

    
           # flatten list of skipped keys and filter-for-installers 
        
           options.skip_keys = [key for s in options.skip_keys for key in s.split(' ')] 
        
           options.filter_for_installers = [inst for s in options.filter_for_installers for inst in s.split(' ')]

ruffsl · 2020-04-06T21:50:48Z

IMO, inputs separated by spaces are bit friendlier to work with in scripts and pipes using the CLI.

dirk-thomas · 2020-04-06T21:58:43Z

inputs separated by spaces are bit friendlier to work with

Btw this is also the only way supported by argparse. So in order to not do custom command line argument parsing I would say we should use what is offered by the "standard" in Python.

mortenfyhn · 2020-04-28T11:12:36Z

Just chiming in that I'm very excited about this feature.

ros-discourse · 2020-05-28T18:11:32Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-noetic-ninjemys-release/14262/9

iluetkeb · 2020-09-22T13:37:20Z

This seems to have stalled a bit -- any particular reason, or can we revive it?

mateus-amarante · 2020-10-04T22:16:51Z

I opened a Pull Request to @nalt's branch (nalt#1) finishing this update. Let's see if he answers. I'm not sure this is the best move in this situation. Alternatively, I could open a new PR but then we would have two PR's for the same thing. Please, let me know if there is a more adequate action.

Rebase + improvements for ros-infrastructure#727

emersonknapp · 2020-10-09T16:41:30Z

Should this have been closed by that PR? Seems like this is probably still open until merged into upstream here

gavanderhoorn · 2020-10-09T16:43:34Z

Getting this merged would make using multi-stage Docker builds finally really scalable (no more manual runtime dependency management 🎉).

What's the major blocker?

nuclearsandwich · 2020-10-09T17:06:10Z

What's the major blocker?

At this point it's review time. I'll schedule it for next week and try my best to meet that appointment.

mateus-amarante

There are some problems with the last merge. The following suggestions might fix most of the problems.

mateus-amarante · 2020-10-09T18:18:42Z

Makefile


 test: testsetup
-	nosetests --with-coverage --cover-package=rosdep2 --with-xunit test
+	nosetests3 --with-coverage --cover-package=rosdep2 --with-xunit test


Suggested change

nosetests3 --with-coverage --cover-package=rosdep2 --with-xunit test

nosetests --with-coverage --cover-package=rosdep2 --with-xunit test

It was my bad. I accidentally committed it.

If changes to the test runners are proposed I'd prefer them in a separate PR for ease of review.

Originally, the tests use nosetests (Python 2 version). I didn't intend to propose a test runner change. I changed it locally to try some stuff and committed it unintentionally. This suggestion reverts this change.

mateus-amarante · 2020-10-09T18:20:20Z

src/rosdep2/main.py


 def _rosdep_main(args):
    # sources cache dir is our local database.
+    global _global_options


Suggested change

global _global_options

This is not needed

mateus-amarante · 2020-10-09T18:20:43Z

src/rosdep2/main.py

+                           'Chose from build, buildtool, build_export, exec, run, test, doc. Default: all except doc.')

    options, args = parser.parse_args(args)
+    _global_options = options


Suggested change

_global_options = options

Not needed

mateus-amarante · 2020-10-09T18:22:46Z

src/rosdep2/lookup.py

        :param sources_loader: (optional) Override SourcesLoader used
            for managing sources.list data sources.
+        :param dependency_types: (optional) List of dependency types.
+            Allowed: "build", "buildtool", "run", "test"


Suggested change

Allowed: "build", "buildtool", "run", "test"

Allowed: "build", "buildtool", "build_export", "run", "test", "exec", "doc"

It also supports build_export, exec, and doc dependencies

Should these docs also link to the specification for dependency types either in catkin_pkg or the ROS REPs defining the package.xml format?

Good point. The doc_depend seems to be introduced only in format 2 ( REP127, schema files ), so maybe it is better not to consider it.

mateus-amarante · 2020-10-09T18:26:47Z

src/rosdep2/main.py

+    global _global_options
+    if options is None:
+        options = _global_options


This is no longer needed

Suggested change

global _global_options

if options is None:

options = _global_options

mateus-amarante · 2020-10-09T18:27:07Z

src/rosdep2/main.py

-def _get_default_RosdepLookup(options):
+_global_options = None
+
+def _get_default_RosdepLookup(options=None):


I think it is better to keep this

Suggested change

def _get_default_RosdepLookup(options=None):

def _get_default_RosdepLookup(options):

mateus-amarante · 2020-10-09T18:27:12Z

src/rosdep2/main.py



-def _get_default_RosdepLookup(options):
+_global_options = None


Suggested change

_global_options = None

_global_options = None

This is not needed

nuclearsandwich

Overall these changes are looking promising. Thank you very much for adding tests along with this new feature.

I generally agree with the review suggestions from @mateus-amarante, would you have the opportunity to respond to that feedback before another round of review?

mateus-amarante · 2020-11-16T15:38:32Z

@nuclearsandwich, if you agree, I can create a new PR from my fork, so that I can finish the updates and you can do another review. I'm very interested in this feature, so I will be committed to fixing any problem pointed out.

nuclearsandwich · 2020-11-16T17:37:29Z

I can create a new PR from my fork, so that I can finish the updates and you can do another review. I'm very interested in this feature, so I will be committed to fixing any problem pointed out.

Please go ahead!

emersonknapp · 2020-12-11T20:35:33Z

@nuclearsandwich i think we can close this PR, #789 is looking good (and could use maintainer review :))

nuclearsandwich · 2021-06-25T16:09:50Z

Replaced by #789

#789) Co-authored-by: Nicolas Alt <nalt@users.noreply.github.com> Co-authored-by: Steven! Ragnarök <steven@nuclearsandwich.com>

Selection of dependency types with cmd line option

d674078

nalt mentioned this pull request Oct 28, 2019

Ability to query build or exec dependencies only #587

Closed

nuclearsandwich mentioned this pull request Apr 2, 2020

doc_depend resolution + unit testing #646

Closed

nuclearsandwich self-assigned this Apr 2, 2020

nuclearsandwich mentioned this pull request Apr 6, 2020

also install buildtool_export_depends #753

Merged

ruffsl mentioned this pull request Apr 25, 2020

[ROS] Add Official Docker image for ROS2 docker-library/docs#1381

Merged

nalt and others added 7 commits October 4, 2020 13:36

Selection of dependency types with cmd line option

078d1cb

Remove global variable + fix errors detected by the tests

c8a2a3d

Consider build_export_depends separately

9c47130

Test RosdepLookup dependency_types argument

65a5221

Add support to exec and doc depends

0b9f7d3

Test -t option for keys()

099c46d

Minor update in help message

3f9bc89

mateus-amarante mentioned this pull request Oct 4, 2020

Rebase + improvements for #727 nalt/rosdep#1

Merged

Merge branch 'master' into update-dep-type-option

9e2d8ad

Merge pull request #1 from mateus-amarante/update-dep-type-option

e2b6273

Rebase + improvements for ros-infrastructure#727

nalt closed this in nalt/rosdep#1 Oct 8, 2020

nuclearsandwich reopened this Oct 9, 2020

mateus-amarante reviewed Oct 9, 2020

View reviewed changes

nuclearsandwich reviewed Nov 12, 2020

View reviewed changes

mateus-amarante mentioned this pull request Nov 16, 2020

Selection of dependency types with cmd line option [#727 continuation] #789

Merged

nuclearsandwich closed this Jun 25, 2021

	nosetests3 --with-coverage --cover-package=rosdep2 --with-xunit test
	nosetests --with-coverage --cover-package=rosdep2 --with-xunit test

	Allowed: "build", "buildtool", "run", "test"
	Allowed: "build", "buildtool", "build_export", "run", "test", "exec", "doc"

	global _global_options
	if options is None:
	options = _global_options

	def _get_default_RosdepLookup(options=None):
	def _get_default_RosdepLookup(options):



		def _get_default_RosdepLookup(options):
		_global_options = None

Conversation

nalt commented Oct 28, 2019

Uh oh!

nuclearsandwich commented Apr 2, 2020

Uh oh!

gavanderhoorn commented Apr 6, 2020

Uh oh!

NikolausDemmel commented Apr 6, 2020

Uh oh!

ruffsl commented Apr 6, 2020

Uh oh!

dirk-thomas commented Apr 6, 2020

Uh oh!

mortenfyhn commented Apr 28, 2020

Uh oh!

ros-discourse commented May 28, 2020

Uh oh!

iluetkeb commented Sep 22, 2020

Uh oh!

mateus-amarante commented Oct 4, 2020

Uh oh!

emersonknapp commented Oct 9, 2020

Uh oh!

gavanderhoorn commented Oct 9, 2020

Uh oh!

nuclearsandwich commented Oct 9, 2020

Uh oh!

mateus-amarante left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateus-amarante Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateus-amarante Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuclearsandwich left a comment

Choose a reason for hiding this comment

Uh oh!

mateus-amarante commented Nov 16, 2020

Uh oh!

nuclearsandwich commented Nov 16, 2020

Uh oh!

emersonknapp commented Dec 11, 2020

Uh oh!

nuclearsandwich commented Jun 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

mateus-amarante Oct 9, 2020 •

edited

Loading

mateus-amarante Nov 12, 2020 •

edited

Loading