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

Implement the use of the generate_export_header cmake macro #43

Merged
merged 4 commits into from
Apr 7, 2017
Merged

Implement the use of the generate_export_header cmake macro #43

merged 4 commits into from
Apr 7, 2017

Conversation

j-rivero
Copy link
Contributor

After discussion in #42 following @traversaro suggestion in #40

I've tested it Ubuntu Xenial with GCC 5.4.0 and Windows7 with MSVC 2013.

This was referenced Mar 23, 2017
@dirk-thomas
Copy link
Member

dirk-thomas commented Mar 27, 2017

FYI ros2/rmw_fastrtps#87 (comment)

@scpeters
Copy link
Contributor

I've been peeking at the cmake source code, and I think this requires cmake 2.8.6 since it exists here:

but not in 2.8.5:

Should we upgrade the cmake version requirement in CMakeLists.txt?

@j-rivero
Copy link
Contributor Author

Should we upgrade the cmake version requirement in CMakeLists.txt?

done in c322c60

@esteve
Copy link
Member

esteve commented Apr 7, 2017

Is there anything pending? The changes look good to me and would help ros2/rmw_fastrtps#87 a lot. Thanks.

@scpeters scpeters self-requested a review April 7, 2017 15:31
@scpeters scpeters merged commit 9ef664d into ros:master Apr 7, 2017
@scpeters
Copy link
Contributor

scpeters commented Apr 7, 2017

Thanks for the ping @esteve; it looks good to me too. Is it enough for ros2 to have this in the master branch, or do we need another release?

@mikaelarguedas
Copy link
Member

master branch is fine 👍 , thanks !

@dirk-thomas
Copy link
Member

For now we can use the master branch. But for future binary releases (targeted for beta 2) this change needs to be available as a Debian package (in Xenial).

@scpeters
Copy link
Contributor

scpeters commented Apr 7, 2017

Well, we can ask @j-rivero if he thinks a 0.3.3 could make it into xenial. Otherwise it could be hosted on our package server or a PPA.

@j-rivero
Copy link
Contributor Author

j-rivero commented Apr 8, 2017

Well, we can ask @j-rivero if he thinks a 0.3.3 could make it into xenial. Otherwise it could be hosted on our package server or a PPA.

Given the situation, I think that we hardly can argue that this fix falls under the Stable Release Update category. If the new version only includes this patch or do not touch the ABI it is fine to host it in any of our repositories. If the current master branch alters the ABI, we can cherry pick this change as a patch on top of the Xenial release.

@esteve
Copy link
Member

esteve commented Apr 10, 2017

@scpeters thanks for merging the PR, this should pave the way for ros2/rmw_fastrtps#87 to be merged as well.

@scpeters
Copy link
Contributor

@j-rivero if I released 0.3.3 today, could it get into Ubuntu zesty?

@j-rivero
Copy link
Contributor Author

@j-rivero if I released 0.3.3 today, could it get into Ubuntu zesty?

A bit late since it is Final Freeze and the policies for Stable Release Updates (SRUs) applied also to this final step.

lsolanka pushed a commit to ros-hunter/console_bridge that referenced this pull request Dec 26, 2017
* Implement the use of the generate_export_header cmake macro

* Changes in build system and header file

* Include export file header in the installation

* Update cmake version required

(cherry picked from commit 9ef664d)
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.

6 participants