Fix building with custom CXXFLAGS and LDFLAGS #15

Merged
merged 5 commits into from Jul 19, 2016

Projects

None yet

2 participants

@chradcliffe
Contributor

I tried to build rprotobuf with a protobuf library install that was in a custom location (so that I had to set CXXFLAGS and LDFLAGS), but I ran into an issue where the environment variables were picked up by the configure script, but at build time the paths were not set properly. I noticed two issues:

  • The CXXFLAGS variable is clobbered in the configure script late in the build process so that the value written to src/Makevars does not preserve the CXXFLAGS as set in the environment at configure time.
  • The LDFLAGS variable is omitted when setting PKG_LIB in configure.in, which means that LDFLAGS similarly doesn't propagate to src/Makevars

After fixing these issues, I was able to successfully build with a custom libprotobuf location.

chradcliffe added some commits Jul 19, 2016
@chradcliffe chradcliffe Fix building with custom CXXFLAGS and LDFLAGS
* The CXXFLAGS variable was previously clobbered by a call to `R CMD
  config CXXFLAGS`. This commit fixes the issue by assigning the output
  of the call to a temporary variable and appending this temporary
  variable to the original CXXFLAGS

* The LDFLAGS variable is now used when setting PKG_LIBS
1b27ce0
@chradcliffe chradcliffe Reverse the revision bump in configure b436e9e
@chradcliffe chradcliffe Restore AC_MSG_ERROR comment f748830
@chradcliffe chradcliffe Adding to ChangeLog 6d11b06
@eddelbuettel eddelbuettel commented on the diff Jul 19, 2016
configure
@@ -4202,7 +4202,8 @@ fi
if test -z "${R_HOME}"; then
as_fn_error $? "Could not determine R_HOME." "$LINENO" 5
fi
-CXXFLAGS=`"${R_HOME}/bin/R" CMD config CXXFLAGS`
+R_CXXFLAGS=`"${R_HOME}/bin/R" CMD config CXXFLAGS`
+CXXFLAGS="${CXXFLAGS} ${R_CXXFLAGS}"
@eddelbuettel
eddelbuettel Jul 19, 2016 edited Owner

That's a good catch.

(Comment belongs to configure.in, of course.)

@eddelbuettel eddelbuettel and 1 other commented on an outdated diff Jul 19, 2016
@@ -4263,7 +4264,7 @@ fi
## now use all these
PKG_CPPFLAGS="${PKG_CPPFLAGS} ${CXXFLAGS} $protobuf_cxxflags"
-PKG_LIBS="${PKG_LIBS} $rcpp_ldflags $protobuf_libs"
+PKG_LIBS="${PKG_LIBS} ${LDFLAGS} $rcpp_ldflags $protobuf_libs"
@eddelbuettel
eddelbuettel Jul 19, 2016 edited Owner

Doesn't that come in from R? May not do harm here, but might get added twice?

(Comment belongs to configure.in, of course.)

@chradcliffe
chradcliffe Jul 19, 2016 Contributor

You're right. I removed the LDFLAGS inclusion from my working copy and it looks as if src/Makevars is populated with the LDFLAGS variable correctly. I've pushed an update that removes the LDFLAGS so that there's no duplication.

@eddelbuettel
Owner

Thanks. Looks like a nicely minimal patch. All this is of course fallback mode because under normal circumstances pkg-config tells us all this ...

@chradcliffe chradcliffe Removed redundant LDFLAGS from configure.in
rcpp_ldflags already contains the environment's LDFLAGS so adding it
again to PKG_LIB isn't necessary.
5b260fe
@eddelbuettel eddelbuettel merged commit 77dd9d7 into eddelbuettel:master Jul 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment