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

Compile libphidget22 with -fPIC #88

Merged
merged 1 commit into from
Mar 27, 2021
Merged

Compile libphidget22 with -fPIC #88

merged 1 commit into from
Mar 27, 2021

Conversation

cottsay
Copy link

@cottsay cottsay commented Mar 26, 2021

This seems to be unnecessary on some platforms, but for greater compatibility, it's best to specify it explicitly.

Should resolve build failures on the buildfarm for RHEL 8: https://build.ros2.org/view/Rbin_rhel_el864/job/Rbin_rhel_el864__libphidget22__rhel_8_x86_64__binary/

This seems to be unnecessary on some platforms, but for greater
compatibility, it's best to specify it explicitly.
@mintar
Copy link
Contributor

mintar commented Mar 26, 2021

Are you sure it will fix those build failures? I don't see any mention of fPIC there, only cross compiling.

That being said, this PR looks good to me. Even if it maybe doesn't fix that problem, it shouldn't hurt.

@clalancette : Feel free to merge if you agree.

@mintar mintar requested a review from clalancette March 26, 2021 10:44
@clalancette
Copy link
Contributor

Honestly, I think something else is going on with that build error. When I try to build libphidget22 by hand in a CentOS 8 container, I can build it just fine without the addition of -fPIC. I'm not sure why that build thinks it is trying to cross-compile, but my guess is that it is something else in the environment that is causing problems.

@cottsay I'm not sure if this is easy to do, but it may be worthwhile to reconfigure that job to spit out the contents of config.log after it fails to see what it thinks the problem is.

@cottsay
Copy link
Author

cottsay commented Mar 26, 2021

...spit out the contents of config.log after it fails...

configure:3289: checking whether we are cross compiling
configure:3297: gcc -o conftest -g -O2 -Wno-incompatible-pointer-types -Wno-deprecated-declarations -Wno-format-truncation  -Wl,-z,relro  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld conftest.c  >&5
/bin/ld: /tmp/ccKF3OWt.o: relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a PIE object; recompile with -fPIC
/bin/ld: final link failed: Nonrepresentable section on output
collect2: error: ld returned 1 exit status
configure:3301: $? = 1
configure:3308: ./conftest
/home/mockbuild/rpmbuild/BUILD/ros-rolling-libphidget22-2.0.2/obj-x86_64-redhat-linux-gnu/libphidget22-src/configure: line 3310: ./conftest: No such file or directory
configure:3312: $? = 127
configure:3319: error: in `/home/mockbuild/rpmbuild/BUILD/ros-rolling-libphidget22-2.0.2/obj-x86_64-redhat-linux-gnu/EP_libphidget22-prefix/src/EP_libphidget22-build':
configure:3321: error: cannot run C compiled programs.
If you meant to cross compile, use `--host'.

The problem is that the CMakeLists.txt is overriding CFLAGS but not LDFLAGS. So the RPM build process sets the platform's standard CFLAGS and LDFLAGS, and it turns out that some of the LDFLAGS depend on the CFLAGS (-fPIC, namely).

This problem could also be solved by appending to the CFLAGS instead of overriding them, or by overriding the LDFLAGS with an empty value.

This change seemed like the easiest pitch.

@clalancette
Copy link
Contributor

Ah, I see. Makes sense, thanks for the explanation. Looks good to me then.

@mintar mintar merged commit 1244f5d into ros-drivers:foxy Mar 27, 2021
@clalancette
Copy link
Contributor

@mintar Do you mind doing a source and bloom release into Foxy and Rolling with this change? Or I can do it if you don't have time. Thanks.

@mintar
Copy link
Contributor

mintar commented Mar 29, 2021

I'm on vacation at the moment, so it would be great if you could do it. In general I think it would be good if you'd do the ROS2 releases, merge PRs etc., and I'll do it for ROS1 if you agree.

@clalancette
Copy link
Contributor

I'm on vacation at the moment, so it would be great if you could do it. In general I think it would be good if you'd do the ROS2 releases, merge PRs etc., and I'll do it for ROS1 if you agree.

Sounds good to me, thank you!

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

3 participants