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

Unreproducable Build Issues with Recent Industrial CI Version #166

Closed
Jmeyer1292 opened this issue May 3, 2017 · 11 comments
Closed

Unreproducable Build Issues with Recent Industrial CI Version #166

Jmeyer1292 opened this issue May 3, 2017 · 11 comments

Comments

@Jmeyer1292
Copy link

Greetings, it's me again, your favorite user. I'm trying to do some work on the Godel project and I'm running into brand new build issues that do not show up on my PC even with a clean install.

So here's the deal:

  • The last commit for Godel was on March 30th. CI passed at that time.
  • Making a new pull request, here, causes ci to fail (see logs) in an untouched package.
  • I was unable to reproduce the issue, so I made a branch from the known good kinetic-devel and started cleaning up all kinds of odd cmake stuff in a new branch (here) with no luck.
  • To test, I created a new branch, also off of kinetic, that checked out your tagged 0.3.3 version and it works. See the logs.

Now for my questions:

  1. What has changed since 0.3.3 that might cause this issue?
  2. If its obvious to you, how might I fix this?
  3. How do I go about debugging situations where catkin clean && catkin build do not reproduce issues I see on travis with industrial_ci? Godel is too damn big and takes 20 minutes to even fail.

Sorry for the trouble. Thanks for making a great tool for our community.

@shaun-edwards
Copy link
Member

I'm sure this is a dumb question, but it says it is missing polygon_utils, here. How is this dependency normally installed?

One of the ways I have seen things fail (after they were working) is when dependencies change. Because we don't rebuild when dependencies change, we can miss breaking changes. Of course, dependencies should be stable, but not always.

@mathias-luedtke
Copy link
Member

mathias-luedtke commented May 3, 2017

@Jmeyer1292:

Thanks for the report!

What has changed since 0.3.3 that might cause this issue?

Most changes are unrelated..
The only important change is that we build to install space by default (#150).
For some reason this breaks your build, since polygon_utils does not get installed, but the generated install files get used.
I will have look at it.
This might be a feature/bug in catkin_tools.

If its obvious to you, how might I fix this?

Add install tags ;)
I have not yet found the underlying problem though.

How do I go about debugging situations where catkin clean && catkin build do not reproduce issues I see on travis with industrial_ci? Godel is too damn big and takes 20 minutes to even fail.

You can set the install option in your workspace, it should fail.
btw, you can run industrial_ci on your system with docker, too! (run_ci script)

@shaun-edwards:

One of the ways I have seen things fail (after they were working) is when dependencies change. Because we don't rebuild when dependencies change, we can miss breaking changes.

Travis introduced a cron job feature :)

@mathias-luedtke
Copy link
Member

This might be a feature/bug in catkin_tools.

I's a feature! (see catkin/catkin_tools#427 (comment))
I strongly encourage you to add proper install rules!
'industrial_ci` is about quality testing as well, and proper install rules should be essential.
However, since this was a unintended change in behaviour, I will try to add a way to opt-out.

@mathias-luedtke
Copy link
Member

I will try to add a way to opt-out.

For now: BEFORE_SCRIPT='catkin config -w $CATKIN_WORKSPACE --no-install'

@Jmeyer1292
Copy link
Author

Jmeyer1292 commented May 3, 2017

Thanks for looking into this. I will try the suggested fix. It is probably not realistic for me to fix the build and install rules for all of the legacy code in our various big projects.

Please do consider an opt out for those of us who never intended to use the install features. Especially if we've already disabled install testing through your existing feature.

@Jmeyer1292
Copy link
Author

I would point out that the catkin wiki still indicates install rules as optional. http://wiki.ros.org/catkin/CMakeLists.txt

@mathias-luedtke
Copy link
Member

mathias-luedtke commented May 3, 2017

I will try the suggested fix

I have already tried it locally, worked for me :)

Especially if we've already disabled install testing through your existing feature.

The install testing is something different, it runs rostest for installed *.test files..

Please do consider an opt out for those of us who never intended to use the install features.

The BEFORE_SCRIPT is a proper fix.
I might implement a shortcut like CATKIN_CONFIG=--no-install

I would point out that the catkin wiki still indicates install rules as optional.

Yes, install rules are optional, but so is CI testing ;)
IMHO industrial-grade software should include install rules (lessons learned..).
Apart from enabling the install, it helps to identify the result artifacts and their dependencies.
The devel space will get removed in ROS2 anyway

(Addtion: The wiki pages are outdated, the best documentation can be found in the API docs)

It is probably not realistic for me to fix the build and install rules for all of the legacy code in our various big projects.

I totally understand your point!
I am sorry, I just wasn't aware of this catkin_tools feature when we merged it, and our test cases do not cover libraries.
We can't revert it anymore, because this would break other repositories that rely on install rule testing.
So for now the BEFORE_SCRIPT (or the to-be-implemented CATKIN_CONFIG) should do the trick for you :)

@Jmeyer1292
Copy link
Author

Well realistically, 99% of our software isn't close to "industrial" grade, so I scoff a bit, but I will make sure that future development follows the rules you outlined.

Out of curiosity though, does the eventual removal of the devel space mean that workspaces can't be built in total isolation from one another from source (e.g. I have the same package in different development states in multiple workspaces)? Or is it just, from the user perspective, a change in book-keeping implementation?

I appreciate the help.

@gavanderhoorn
Copy link
Member

Out of curiosity though, does the eventual removal of the devel space mean that workspaces can't be built in total isolation from one another from source (e.g. I have the same package in different development states in multiple workspaces)

If I understand you correctly: there will still be (or: there can still be) multiple install spaces (one per workspace fi). It's just that the devel space (which is a weird sort of trick to make catkin play nice with rosbuild in a way) will not be used anymore.

@Jmeyer1292
Copy link
Author

Thanks for the suggestions. I'm closing this.

@mathias-luedtke
Copy link
Member

Ths issue is now tracked in #176

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

No branches or pull requests

4 participants