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

HPCC-17340 Include libRInside.so and check for Rcpp version at build #9791

Merged

Conversation

@Michael-Gardner
Copy link
Contributor

commented Mar 31, 2017

Signed-off-by: Michael Gardner michael.gardner@lexisnexis.com

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browers
    • The component(s) render as expected

Testing:

@hpcc-jirabot

This comment has been minimized.

Copy link

commented Mar 31, 2017

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2017

@xwang2713 Please review this. Richard wanted libRInside.so added to the rembed package. I've gone ahead and fixed the build boxes so they all have appropriate versions of R, Rcpp, Rinside, and inline. This also checks at build time to make sure the Rcpp version is equal or greater than 0.12.1.

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2017

@richardkchapman Here are the changes you requested in #9755 for the build/packaging process.

@Michael-Gardner Michael-Gardner force-pushed the Michael-Gardner:HPCC-17340 branch from ed20c77 to 9db1a58 Apr 3, 2017
@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2017

@richardkchapman I've gotten rinside included into the package, but in testing I found that r-base-core wasn't being added as a dependency even when using dpkg-shlibdeps. I've gone ahead and appended it to the dependency list within the Rembed project.

In testing on a 16.04 machine with no dependencies met, apt-get install -f now pulls in libr.so. Our copy of libRinside.so under /opt/HPCCSystems/lib is getting linked to correctly. But when run eclcc against testing/regress/embedR.ecl, the output executable gives this error when ran.

Error: function 'dataptr' not provided by package 'Rcpp'
In addition: Warning message:
In library(package, lib.loc = lib.loc, character.only = TRUE, logical.return = TRUE, :
there is no package called 'Rcpp'
Segmentation fault (core dumped)

I checked with the rembed.cpp and it does appear that RCPP_HEADER_ONLY is defined properly. If I look at the symbol table of libRembed.so I see HIDE_RCPP_dataptr defined in text. And no symbols that are just dataptr. Also all the symbols in libRembed.so that are in the Rcpp namespace appear to be defined in text, and there are none that are undefined (which is expected bc we aren't linking to a libRcpp.so).

@richardkchapman

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

@Michael-Gardner R versioning and Rcpp is a mess.

You could try adding r-cran-rcpp to the dependencies as well as r-cran-core. But I am concerned that may interfere with people that want to install R/Rcpp manually in order to get newer versions?

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2017

It looks like we might be good post 16.04 if we include r-cran-rcpp. 14.04 and 12.04 are both going to have issues with rcpp versioning but if those are really the only two hard cases, we can probably get away with adding something to the redbook about them. It will make the downstream installation of the Rembed plugin easier for the majority of users.

el6 and el7 look like the epel repositories will provide sufficient versions of rcpp. el5 might not have a sufficient version but we're just about out of the support window for that version of centos.

I'll also test against installing Rcpp through the package manager, and then a higher version through a manual install.

@Michael-Gardner Michael-Gardner force-pushed the Michael-Gardner:HPCC-17340 branch from 9db1a58 to ed4c4de Apr 4, 2017
Signed-off-by: Michael Gardner <michael.gardner@lexisnexis.com>
@Michael-Gardner Michael-Gardner force-pushed the Michael-Gardner:HPCC-17340 branch from ed4c4de to bb8928b Apr 7, 2017
@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2017

@richardkchapman Ready for you to look at. Rembed now appends its dependencies. It also includes libRInside.so in the package.

Ubuntu 12.04 was the only platform I was having any issues with. Mainly due to the fact that the version of R on it was so old and we had to use a custom repository to pull in the dependencies. But since that won't be a supported version for 6.4.0, there no longer is a problem.

@HPCCSmoketest

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

Automated Smoketest:
Sha: bb8928b
Build: success
ECL Watch: Rebuilding Site

errors warnings build time
0 65 92.765 seconds

Install hpccsystems-platform-community_6.3.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test total passed failed errors timeout
unittest 93 93 0 0 0
wutoolTest(Dali) 19 19 0 0 0
wutoolTest(Cassandra) 19 19 0 0 0

Regression test result:

phase total pass fail
setup (hthor) 10 10 0
setup (thor) 10 10 0
setup (roxie) 10 10 0
test (hthor) 721 721 0
test (thor) 617 617 0
test (roxie) 746 746 0

HPCC Stop: OK
HPCC Uninstall: OK

@richardkchapman richardkchapman merged commit a6d31b8 into hpcc-systems:candidate-6.4.0 Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.