Add ROSDEP_INSTALL_OPTS env #180

wkentaro · 2017-06-03T16:09:28Z

I need this to use package that have bad rosdep keys in UPSTREAM_WORKSPACE by -r to rosdep install.

I need this to use package that have bad rosdep keys as UPSTREAM_WORKSPACE with `-r` or unsetting `-i`.

130s · 2017-06-05T20:30:18Z

industrial_ci/src/tests/source_tests.sh

@@ -126,8 +126,11 @@ fi

 ici_time_start rosdep_install

+if [ "${ROSDEP_INSTALL_OPTS// }" == "" ]; then


This if clause only defines a variable. So can it be moved to industrial_ci/src/env.sh?
Cc @ipa-mdl

130s

A few things.

Just trying to understand,

I need this to use package that have bad rosdep keys in UPSTREAM_WORKSPACE by -r to rosdep install.

The situation you're trying to address is:
- Packages installed via rosdep is not what you want. Instead you want to build from source in your workspace.
Is this correct? Your statement bad rosdep keys is a bit unclear to me.
Would you mind adding a job to test the new variable?

Also I'd like to ask @ipa-mdl for the review.

mathias-luedtke · 2017-06-05T21:36:45Z

I need this to use package that have bad rosdep keys in UPSTREAM_WORKSPACE by -r to rosdep install.

So why do you need to overwrite -q and --ignore-src in this case?
If an upstream package is broken, it would be great if you report or even fix it!

130s · 2017-06-09T16:59:20Z

So why do you need to overwrite -q and --ignore-src in this case?

I can't speak for @wkentaro and just am personally trying to think of how useful it can be.
When upstream binary is unavailable (even for temporarily) but you want to somehow proceed your CI jobs, this change might be useful (regardless those jobs can pass or not) to work around the situation.

Anyways, generally speaking isn't it a good idea to give users an option to pass options to external commands like rosdep?

wkentaro · 2017-06-09T17:16:01Z

Sorry for late.

The situation you're trying to address is:

Packages installed via rosdep is not what you want. Instead you want to build from source in your workspace.

Is this correct? Your statement bad rosdep keys is a bit unclear to me.

Sorry for unclear comment, I mean deprecated or unreleased dependencies for "bad rosdep keys".
In that case, I want to pass options -r or --skip-keys=xxx to rosdep.
(My lab project has default option of -r because there are some unreleased or deprecated packages: https://github.com/jsk-ros-pkg/jsk_travis#environmental-variables)

So why do you need to overwrite -q and --ignore-src in this case?

In this case, it is not needed.
But when someone wants overwrite -q or --ignore-src in the future, it will need to break the api or behavior if I don't set -q --ignore-src as the default now.

mathias-luedtke · 2017-06-29T12:11:22Z

Sorry for unclear comment, I mean deprecated or unreleased dependencies for "bad rosdep keys".

Deprecated and unreleased packages should not lead to "bad rosdep keys".
(assuming that you provide the unreleased packages from source)

But when someone wants overwrite -q or --ignore-src in the future, it will need to break the api or behavior if I don't set -q --ignore-src as the default now.

I can't imaging any use case for disabling --ignore-src for CI tests.
-q is superfluous anyway, but it does not hurt.

wkentaro · 2017-06-29T15:31:40Z

I can't imaging any use case for disabling --ignore-src for CI tests.

I also think this is rare, but what about below case?

you want to use a package (package_A) in a github repository which includes some packages (package_A,B,C), so you put it in .travis.rosinstall.
there is no dependency among package_A,B,C
you want to build only package_A

In this case, you need to disable --ignore-src, to use package_B, package_C installed by apt.

mathias-luedtke · 2017-06-29T17:58:57Z

@wkentaro wrote:

In this case, you need to disable --ignore-src, to use package_B, package_C installed by apt.

rosdep would install them by apt, but cakin will still use the package from .travis.rosinstall.
(I have added a blacklist feature for rosdep AND catkin in #137)
For now it would be better to remove or disable the packages in a BEFORE_SCRIPT.

@130s wrote:

Anyways, generally speaking isn't it a good idea to give users an option to pass options to external commands like rosdep?

IMHO rosdep doesn't have any additional, useful flags that make sense for CI.
If we introduce this option now, #137 does not work anymore as this option would undermine the new features.

wkentaro · 2017-06-29T23:41:39Z

rosdep would install them by apt, but cakin will still use the package from .travis.rosinstall.
(I have added a blacklist feature for rosdep AND catkin in #137)
For now it would be better to remove or disable the packages in a BEFORE_SCRIPT.

Maybe you misunderstand my needs, and I don't need to disable the --ignore-src option, but -r.
I just added --ignore-src to the env variable for future possible changes, because we can ignore being built packages by catkin --blacklist as you said.

mathias-luedtke · 2017-06-30T08:49:06Z

Maybe you misunderstand my needs, and I don't need to disable the --ignore-src option, but -r.

I strongly encourage you to not use -r, but --skip-keys "package_X, package_Y, package_Z" for CI tests.
rosdep can fail for various reasons and ignoring them leads to broken builds.

What do you think of introducing a ROSDEP_SKIP_KEYS variable?
I could support this in #137 as well :)

I just added --ignore-src to the env variable for future possible changes, because we can ignore being built packages by catkin --blacklist as you said.

You can blacklist the build, but rostest will use the packages from source space (I guess).

wkentaro · 2017-06-30T10:14:50Z

What do you think of introducing a ROSDEP_SKIP_KEYS variable?
I could support this in #137 as well :)

I agree that.

mathias-luedtke · 2017-06-30T11:41:47Z

What do you think of introducing a ROSDEP_SKIP_KEYS variable?

I agree that.

here we go: #184
Does this fit your needs?

wkentaro · 2017-06-30T15:42:48Z

Yeah, looks good.

…

On Fri, Jun 30, 2017 at 8:41 PM, Mathias Lüdtke ***@***.***> wrote: What do you think of introducing a ROSDEP_SKIP_KEYS variable? I agree that. here we go: #184 <#184> Does this fit your needs? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#180 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEHFk_h3welzAccYkONffbiAn_E13fkJks5sJN77gaJpZM4NvIFJ> .

-- 和田健太郎 / Kentaro Wada http://wkentaro.com

130s · 2017-07-05T22:40:32Z

Anyways, generally speaking isn't it a good idea to give users an option to pass options to external commands like rosdep?
IMHO rosdep doesn't have any additional, useful flags that make sense for CI.
If we introduce this option now, #137 does not work anymore as this option would undermine the new features.

I'm not sure about this yet because:

While trying to understand [WiP] Fix upstream downstream package logic #137, I'm stuck at Upstream/downstream logic design #106 (comment) so can't comment on [WiP] Fix upstream downstream package logic #137 yet.
Not sure how much/little rosdep commands we want to expose to end users. I often use some of them, e.g. -y Tell the package manager to default to y or fail when installing.

That said, #184 itself looks good so I merged it.

mathias-luedtke · 2017-07-11T19:17:04Z

I will close this for now, skipping keys was implemented in #184

130s · 2017-07-11T19:29:43Z

Yeah, fair enough. Besides, thanks for the discussion @wkentaro @ipa-mdl

Add ROSDEP_INSTALL_OPTS env

4ef1f7b

I need this to use package that have bad rosdep keys as UPSTREAM_WORKSPACE with `-r` or unsetting `-i`.

wkentaro force-pushed the rosdep_install_opts branch from fd3a093 to 4ef1f7b Compare June 3, 2017 16:14

130s reviewed Jun 5, 2017

View reviewed changes

130s requested changes Jun 5, 2017

View reviewed changes

130s requested a review from mathias-luedtke June 5, 2017 20:37

mathias-luedtke closed this Jul 11, 2017

wkentaro deleted the rosdep_install_opts branch August 26, 2017 15:44

RyodoTanaka mentioned this pull request Jan 29, 2018

Travisのビルドが通らない！！ Nishida-Lab/motoman_project#109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ROSDEP_INSTALL_OPTS env #180

Add ROSDEP_INSTALL_OPTS env #180

wkentaro commented Jun 3, 2017 •

edited

Loading

130s Jun 5, 2017

130s left a comment •

edited

Loading

mathias-luedtke commented Jun 5, 2017

130s commented Jun 9, 2017

wkentaro commented Jun 9, 2017 •

edited by 130s

Loading

mathias-luedtke commented Jun 29, 2017

wkentaro commented Jun 29, 2017 •

edited

Loading

mathias-luedtke commented Jun 29, 2017 •

edited

Loading

wkentaro commented Jun 29, 2017

mathias-luedtke commented Jun 30, 2017

wkentaro commented Jun 30, 2017

mathias-luedtke commented Jun 30, 2017

wkentaro commented Jun 30, 2017 via email

130s commented Jul 5, 2017

mathias-luedtke commented Jul 11, 2017

130s commented Jul 11, 2017

		@@ -126,8 +126,11 @@ fi

		ici_time_start rosdep_install

		if [ "${ROSDEP_INSTALL_OPTS// }" == "" ]; then

Add ROSDEP_INSTALL_OPTS env #180

Add ROSDEP_INSTALL_OPTS env #180

Conversation

wkentaro commented Jun 3, 2017 • edited Loading

130s Jun 5, 2017

Choose a reason for hiding this comment

130s left a comment • edited Loading

Choose a reason for hiding this comment

mathias-luedtke commented Jun 5, 2017

130s commented Jun 9, 2017

wkentaro commented Jun 9, 2017 • edited by 130s Loading

mathias-luedtke commented Jun 29, 2017

wkentaro commented Jun 29, 2017 • edited Loading

mathias-luedtke commented Jun 29, 2017 • edited Loading

wkentaro commented Jun 29, 2017

mathias-luedtke commented Jun 30, 2017

wkentaro commented Jun 30, 2017

mathias-luedtke commented Jun 30, 2017

wkentaro commented Jun 30, 2017 via email

130s commented Jul 5, 2017

mathias-luedtke commented Jul 11, 2017

130s commented Jul 11, 2017

wkentaro commented Jun 3, 2017 •

edited

Loading

130s left a comment •

edited

Loading

wkentaro commented Jun 9, 2017 •

edited by 130s

Loading

wkentaro commented Jun 29, 2017 •

edited

Loading

mathias-luedtke commented Jun 29, 2017 •

edited

Loading