Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Message dependencies do not update when a dependent message's dependencies change #41

Closed
brindza opened this Issue · 9 comments

4 participants

@brindza

I think I said that right in the title.

Basically there is a case when the message generation is not rerun even when it should be. The problem is the list of .msg dependencies is generated during the cmake configure step and if you add a new field to a .msg file that is another message type that dependency list is never updated (until you rerun the cmake step).

You can reproduce the problem as follows.

  1. Create a message package with two messages.

    a1.msg:

     int32 a1
    

    a2.msg:

     int32 a2
    
  2. Run catkin_make. It will generate a1.h and a2.h.

  3. In a1.msg add an field that is of type a2.

    a1.msg:

     int32 a1
     a2 a2_msg
    
  4. Run catkin_make. a1.h is regenerated correctly.

  5. Add any field to a2.msg.

    a2.msg:

     int32 a2
     float32 a2_float
    
  6. Run catkin_make. a2.h is regenerated correctly. However a1.h is not. You can look at the "Definition" in a1.h and see it is missing the new field "a2_float". The md5sum is also wrong. If you delete the build directory and do a clean build the md5sum for a1 is now different.

@dirk-thomas
Owner

I can confirm the described behavior. Whenever a dependency to other messages is introduced a CMake reconfigure is necessary.

There are two possible fixes for this:

  • stamp() each message/service file which will trigger a reconfigure every time one of those files is touched. The patch for that would be adding the following to pkg-genmsg.cmake.em:
@{all_deps = dict(msg_deps.items() + srv_deps.items())}
# stamp all message/service files to trigger reconfigure on change
@[for f in all_deps.keys()]@
get_filename_component(filename "@(f)" NAME)
configure_file("@(f)"
  ${CMAKE_CURRENT_BINARY_DIR}/catkin_generated/genmsg_stamps/${PROJECT_NAME}/${filename}.stamp
  COPYONLY)
@[end for]@# messages and services
  • detect the change of dependencies at make-time, print a reasonable error message telling that the user has to rerun cmake and error out

While version one is easy to implement it results in a significant overhead for reconfiguring in a lot of cases where it would not be necessary.

The second version takes more effort to implement and will take a constant time during make to perform that check (invoking another Python script for each message/service file). But the big advantage will be that reconfiguring will only needed when absolutely necessary. I have implemented this in PR #42. Please consider testing it and providing feedback if it works for you.

@dirk-thomas dirk-thomas self-assigned this
@dirk-thomas dirk-thomas added the bug label
@dirk-thomas
Owner

The latest release of genmsg (0.5.1) failed downstream packages because of the implementation of version two in #42 (e.g. http://jenkins.ros.org/view/IbinT64/job/ros-indigo-std-srvs_binarydeb_trusty_i386/8/console).

The problem is that during the install step we currently do not source the setup file. But since install depends on all targets it tries to run the dependency check command which requires genmsg to be available in Python. (Potentially install can perform exactly the same as a build.)

We have two options here:

  • fall back to implement version one with the obvious disadvantages stated above

  • update the Debian rule file generated by bloom to also source the setup before performing an installation (patching only the genmsg gbp is not sufficient since it is required for every downstream package using genmsg).

@esteve @tfoote @wjwwood What do you think?

@dirk-thomas dirk-thomas reopened this
@tfoote
Owner

If we're already exposing the setup file to the make command. We should do the same for the install command since it usually inherently invokes the make command. This will also resolve the bloom issue for gradle. So I'd suggest we do the debuan rule file update.

@dirk-thomas
Owner

I would agree to this. I we want to go this route we should do it soonish because all affected indigo packages will need to be re-released - and the number is growing every day.

@wjwwood
Owner

Is the patch proposed in ros-infrastructure/bloom#204 appropriate? I can just merge that one if it is.

@dirk-thomas
Owner

It overrides the install command and only adds the sourcing of the setup file if it is available (like it does for all the other targets). So yes, it looks to me that it does exactly the right thing.

Should I try do some indigo releases with this patch locally applied to validate that it actually makes the genmsg stuff pass on the farm?

@wjwwood
Owner

That sounds like a good idea, if it works then I can merge it and release a new version of bloom.

@dirk-thomas
Owner

Patched bloom version worked for:

@wjwwood Please go ahead with a bloom patch release containing the PR (with tabs).

This ticket will be closed again - we stick to version two and change the Debian rule file instead.

@dirk-thomas dirk-thomas closed this
@severin-lemaignan severin-lemaignan referenced this issue from a commit in severin-lemaignan/robotpkg
Anthony Mallet [middleware/ros-genmsg] Update to indigo (0.5.3)
Changes since 0.4.20:

0.5.3 (2014-07-10)
------------------
* escape messages to avoid CMake warning (`#49
<ros/genmsg#49)

0.5.2 (2014-05-07)
------------------
* refactor to generate pkg-msg-paths.cmake via configure_file() instead of empy
(`#43 <ros/genmsg#43)
* fix python 3 compatibility (`#45 <ros/genmsg#45)
* remove debug message introduced in 0.5.1 (`#42
<ros/genmsg#42)

0.5.1 (2014-03-04)
------------------
* add check for changed message dependencies (`#41
<ros/genmsg#41)

0.5.0 (2014-02-25)
------------------
* remove usage of debug_message() (`#40
<ros/genmsg#40)

0.4.24 (2014-01-07)
-------------------
* python 3 compatibility (`#36 <ros/genmsg#36,
`#37 <ros/genmsg#37)
* add support for ROS_LANG_DISABLE env variable (`ros/ros#39
<ros/ros#39)
* fix installation of __init__.py from devel space (`#38
<ros/genmsg#38)

0.4.23 (2013-09-17)
-------------------
* fix installation of __init__.py file for packages where name differs from
project name (`#34 <ros/genmsg#34)
* rename variable 'config' to not collide with global variable (`#33
<ros/genmsg#33)
* fix service files variable to only contain package relative paths (`#32
<ros/genmsg#32)

0.4.22 (2013-08-21)
-------------------
* make genmsg relocatable (`ros/catkin#490
<ros/catkin#490)
* add warning in case generate_messages() is invoked without any messages and
services (`#31 <ros/genmsg#31)
* check if files have been generated before trying to install them (`#31
<ros/genmsg#31)

0.4.21 (2013-07-03)
-------------------
* check for CATKIN_ENABLE_TESTING to enable configure without tests
84d4528
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.