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

update travis.yml #6

Merged
merged 21 commits into from Apr 4, 2019
Merged

Conversation

k-okada
Copy link
Contributor

@k-okada k-okada commented Apr 3, 2019

@k-okada
Copy link
Contributor Author

k-okada commented Apr 3, 2019

@d-nakamichi
Copy link
Contributor

Thanks for your help!
Now I activated GitHub - TravisCI integration.

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Apr 3, 2019

@k-okada: did you have any particular reason not to use industrial_ci? It's not a panacea, but leads to very readable and simple .travis.yml files (see this one as an example).


Edit: test build with industrial_ci: test build (takes approx 10-ish minutes per build config matrix row).

@k-okada k-okada closed this Apr 3, 2019
@k-okada k-okada reopened this Apr 3, 2019
gavanderhoorn and others added 16 commits April 3, 2019 08:51
Tries to load xacro as parameter.
Tries to load xacros as parameters.
'khi_robot_msgs' is already a build_depend and is another package, so build ordering will already take the dependencies into account.
The main library is not exported (by catkin_package(..)), so the headers also don't need to be installed.

This also resolves an installation issue: the install(..) statement expects a sub dir called 'khi_robot_control' in the 'include' directory, but that doesn't exist.
@gavanderhoorn
Copy link
Contributor

@k-okada: it appears you merged an older version of master...gavanderhoorn:pkging_updates.

@k-okada
Copy link
Contributor Author

k-okada commented Apr 3, 2019

@gavanderhoorn yes, i know. I just want travis to answer

@k-okada: could you verify this also works for you?

question.

Please wait at most 16 hours, I will finalize this PR and ready to merge and release working version so that we can put khi_robot to the this week's sync.

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Apr 3, 2019

Your proposed travis config is not working because it also includes an Indigo build. The IKFast plugins cannot build under Indigo.

@gavanderhoorn yes, i know. I just want travis to answer

@k-okada: could you verify this also works for you?

question.

My question was specifically about gavanderhoorn@6641def. Your add_travis branch does not contain that commit.

I'm also unsure whether Travis can catch linking issues with dynamic libraries, as there are currently no tests, so no binaries are ever run.


I've run a quick Travis test here with an up-to-date master...gavanderhoorn:pkging_updates and that seems to succeed.

k-okada and others added 2 commits April 3, 2019 22:39
Based on suggestion from @k-okada.

Not sure whether this is an actual proper solution, or just a work-around.

Systems with stricter ld library path security might not like this.
@d-nakamichi
Copy link
Contributor

@k-okada @gavanderhoorn

  • industrial_ci
  • prerelease.sh

Which is better?
For testing just build/install package, industrial_ci seems better because it can select prerelease or not.
Run ROS Prerelease Test

@k-okada
Copy link
Contributor Author

k-okada commented Apr 4, 2019

@d-nakamichi @matsui-hiro @gavanderhoorn Although 2c0a73e did not failed as I expected, but I think this PR is ready to merge and release.

==

@k-okada: did you have any particular reason not to use industrial_ci?

  1. ros_buildfarm is less likely to loose maintainer, If ros_buildfarm is orphaned and we have no alternatives, that’s mean the end of ROS. For example, every time we have new ROS distro or new Ubuntu version, we can assume it is OSRF jobs to update the ros_buildfirm to support them. But as for industrial_ci, someone have to do that. (In my opinion, if ROS-I have the resource to maintain industrial_ci that effort should goes to maintain image_pipeline, rqt, ros-drivers,,, and so on...)

  2. My understanding of industrial_ci is customizable, (At least originally, because that is originally coms from our http://github.com/jsk-ros-pkg/jsk_travis ) So when the source tree did not pass the travis we have opetion to customize the CI parts, for example skip some package, run extra scripts and so on...., and it is good compromise between stability vs development speed, for research labs. But for industrial application, stability is much important. So if we do not pass the travis, we should fix source tree. (So ideally, we should run build script, in addition to prerelease, because our intrest is if the source tree is able to run on the buildfirm.)

@d-nakamichi
Copy link
Contributor

OK. khi_robot uses prerelease at the begging.
I'll accept this PR.
Thanks! @k-okada @gavanderhoorn

@d-nakamichi d-nakamichi merged commit e2366fe into Kawasaki-Robotics:master Apr 4, 2019
@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Apr 4, 2019

@d-nakamichi @k-okada: this PR included a lot of mostly my commits. I would have appreciated it if #5 could have been merged instead. Now changes to the packaging and build setup are merged from a PR that is unrelated (ie: one that "update[s] travis.yml"), which is not good maintainership practice.

This cannot be fixed now any more, but in the future please do not do this with my pull requests.

My apologies, the github UI confused me. It seems both PRs were merged.

@gavanderhoorn
Copy link
Contributor

@k-okada: did you have any particular reason not to use industrial_ci?

1. ros_buildfarm is less likely to loose maintainer, If ros_buildfarm is orphaned and we have no alternatives, that’s mean the end of ROS. For example, every time we have new ROS distro or new Ubuntu version, we can assume it is OSRF jobs to update the ros_buildfirm to support them. But as for industrial_ci, someone have to do that.

I don't really follow your argument here.

Are you worried about whether industrial_ci will be maintained in the future?

(In my opinion, if ROS-I have the resource to maintain industrial_ci that effort should goes to maintain image_pipeline, rqt, ros-drivers,,, and so on...)

I guess we don't agree here: ROS-Industrial is not responsible for maintaining all sorts of packages. Resources are limited everywhere, so we have to make choices.

industrial_ci fulfils a direct need of ROS-Industrial members, so it makes sense to spend effort there.

2. My understanding of industrial_ci is customizable, (At least originally, because that is originally coms from our [http://github.com/jsk-ros-pkg/jsk_travis](https://github.com/jsk-ros-pkg/jsk_travis) ) So when the source tree did not pass the travis we have opetion to customize the CI parts, for example skip some package, run extra scripts and so on...., and it is good compromise between stability vs development speed, for research labs. But for industrial application, stability is much important. So if we do not pass the travis, we should fix source tree.  (So ideally, we should run build script, in addition to prerelease, because our intrest is if the source tree is able to run on the buildfirm.)

re: customizable -> for research labs: I don't fully understand your reasoning here. industrial_ci supports running prereleases as well as many other things. Changing .travis.yml for industrial_ci has to go through the same process (ie: a pull request that should be reviewed) as with the current setup.

What you write are maintenance policy / development process concerns. All software is customisable, and I doubt industrial_ci is any more customisable than the current .travis.yml. In either case: no maintainer can just go in and chance CI configuration, unless the repository has been badly configured.

gavanderhoorn added a commit to gavanderhoorn/khi_robot that referenced this pull request Apr 4, 2019
@k-okada k-okada deleted the add_travis branch April 4, 2019 07:32
@k-okada
Copy link
Contributor Author

k-okada commented Apr 4, 2019

@gavanderhoorn Then, what is the reason for having industrial_ci? in addition to the prerelease.py ?

@gavanderhoorn
Copy link
Contributor

@gavanderhoorn Then, what is the reason for having industrial_ci? in addition to the prerelease.py ?

I believe it would a bit too much to discuss this in a comment.

I'd refer you to the readme and the rest of the documentation.

For me personally, readability of the travis configuration is a big plus. Compare: this with this (if pinning of industrial_ci is a concern, specify a version here).

Please note that I don't feel strongly about this. If you're happy with what you're using right now then please keep using that :)

@k-okada
Copy link
Contributor Author

k-okada commented Apr 4, 2019 via email

d-nakamichi pushed a commit that referenced this pull request Apr 5, 2019
Seems to have been remove in #6.
@gavanderhoorn gavanderhoorn mentioned this pull request Apr 12, 2019
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.

None yet

4 participants