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.

Sign up for GitHub

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

Jump to bottom

Ensure relocatable config files #129

Merged
merged 7 commits into from
Feb 4, 2021
Merged

Ensure relocatable config files #129

merged 7 commits into from
Feb 4, 2021

Conversation

Pro
Copy link
Contributor

@Pro Pro commented Nov 17, 2020 •
edited
Loading

Using CMAKE_INSTALL_PREFIX causes build issues when you relocate the installed CMake package, i.e., as it is done in conan.

CMAKE_INSTALL_PREFIX should not be used in the cmake.in files directly. The @PACKAGE_INIT@ macro initializes PACKAGE_PREFIX_DIR correspondingly.

See also:
gazebosim/gz-tools#30
gazebosim/sdformat#419
gazebosim/gazebo-classic#2782

cc @stefanbuettner

@Pro Pro requested a review from mxgrey as a code owner November 17, 2020 12:29
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Nov 17, 2020
@Pro Pro force-pushed the cmake-relocatable branch from ad0485e to 3eb6828 Compare November 17, 2020 12:31
This was referenced Nov 17, 2020
Merged
Merged
@Pro
fix: Use PACKAGE_PREFIX_DIR to ensure relocatable CMake install for c…
be2ca90 
…onan

Signed-off-by: Stefan Profanter <stefan.profanter@agile-robots.com>
@Pro Pro force-pushed the cmake-relocatable branch from 3eb6828 to be2ca90 Compare November 17, 2020 14:41
scpeters
scpeters requested changes Nov 23, 2020
View reviewed changes
cmake/pkgconfig/ignition.pc.in Outdated
@@ -1,4 +1,4 @@
prefix=@CMAKE_INSTALL_PREFIX@
prefix=${pcfiledir}/../..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed in dartsim/dart#1527 (comment), this relative path is incorrect on Ubuntu

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the .pc.in templates for downstream ignition packages in 1e39fa0 and the .pc file for ign-cmake2 itself in b95d20f

@scpeters
Copy link
Member

scpeters commented Jan 8, 2021

I think it's good now with my fixes, based on the approach in dartsim/dart#1529 and gazebosim/sdformat#419, but I'd like another set of eyes and some testing if possible

cc @j-rivero @caguero

@scpeters scpeters changed the title fix: Use PACKAGE_PREFIX_DIR to ensure relocatable CMake install for conan Use PACKAGE_PREFIX_DIR to ensure relocatable config files Jan 20, 2021
@scpeters scpeters changed the title Use PACKAGE_PREFIX_DIR to ensure relocatable config files Ensure relocatable config files Jan 20, 2021
@chapulina chapulina requested review from j-rivero and caguero January 30, 2021 02:45
@scpeters
Copy link
Member

scpeters commented Feb 4, 2021

perhaps we can test this by including it in a prerelease (see #147)

@scpeters
Copy link
Member

scpeters commented Feb 4, 2021

I'm going to merge this now and include in a prerelease to get further testing

@scpeters scpeters merged commit b03ca34 into gazebosim:ign-cmake2 Feb 4, 2021
scpeters added a commit to scpeters/ign-cmake that referenced this pull request Feb 4, 2021
@scpeters
Changelog for gazebosim#129
03c15b2 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@j-rivero
Copy link
Contributor

j-rivero commented Feb 4, 2021 •
edited
Loading

I'm going to merge this now and include in a prerelease to get further testing

You were a bit faster merging than I was commenting: I tried this today, seems to work fine for all the scenarios I tried: colcon workspace to build ign-math7, simulate a release in Debian packaging system (using -DCMAKE_INSTALL_LIBDIR=lib/aarch64-linux-gnu, would be great to set a test for this) or just manually inspecting the file with the default no-arguments. I see no reason not to merge this one. Nice work.

@scpeters
Copy link
Member

scpeters commented Feb 4, 2021

I'm going to merge this now and include in a prerelease to get further testing

You were a bit faster merging than I was commenting: I tried this today, seems to work fine for all the scenarios I tried: colcon workspace to build ign-math7, simulate a release in Debian packaging system (using -DCMAKE_INSTALL_LIBDIR=lib/aarch64-linux-gnu, would be great to set a test for this) or just manually inspecting the file with the default no-arguments. I see no reason not to merge this one. Nice work.

sorry for not waiting! we can see how the prerelease goes

@stefanbuettner stefanbuettner mentioned this pull request Feb 12, 2021
Open
@scpeters scpeters mentioned this pull request Feb 25, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters approved these changes

@mxgrey mxgrey Awaiting requested review from mxgrey

@j-rivero j-rivero Awaiting requested review from j-rivero j-rivero is a code owner

@caguero caguero Awaiting requested review from caguero

Assignees

@scpeters scpeters

Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Pro @scpeters @j-rivero @chapulina