Dependencies issue #27

FabrizioSandri · 2022-08-30T19:42:30Z

Following a conversation with my mentor @tdhock, we determined that RcppDeepState should not execute the install.packages function within the deepstate_create_makefile method since it is bad practice.

Furthermore, running install.packages does not install the package dependencies, resulting in the shared object file not being generated. The error message for the hermiter package was

ERROR: dependency 'BH' is not available for package 'hermiter'

Instead of terminating, RcppDeepState kept running, consequently compiling the src/*.cpp with weird include problems. To solve this issue, I replaced the installation command with devtools::install and moved it to the action code. Furthermore, I modified the conditions such that if the shared object is not generated, an error is generated and the execution is terminated.

Related to:

FabrizioSandri · 2022-08-30T20:37:45Z

Stopping if the shared object files are missing is not the best option. Instead, if they are absent, we should try to create them first and then quit if they are still not there. This is what I just did with my most recent commit.

Proof of concept

To construct the shared object, the user must call the deepstate_pkg_create method. In certain circumstances, however, the user analyzes a single function rather than the complete package. The shared objects are not created in this situation. For instance, the user can execute

└─RcppDeepState::deepstate_fun_create(path, "rcpp_write_index_outofbound")
  └─RcppDeepState::deepstate_create_makefile(package_path, function_name)

The shared object will not be constructed in this situation. To address this issue, I included another check in the deepstate_create_makefile that attempts to recreate the shared object if it is missing. If they are not present, the execution will be terminated with an error.

tdhock · 2022-08-30T23:23:00Z

I don't understand why the shared object/so file would ever be absent, if the package was installed? Can you clarify / explain what files are created in your rcpp_write_index_outofbound example? Shouldn't there be a shared object created for whatever package that function is defined in?

FabrizioSandri · 2022-08-31T08:13:51Z

To clarify, if a user just wants to analyze a single function rather than the entire package, he may use the deepstate_fun_create function to create the test harness, deepstate_fuzz_fun to compile it, and deepstate_analyze_fun to analyze it.

Example: suppose a user wants to analyze rcpp_write_index_outofbound of the testSAN package without analyzing the entire package. The user performs the following steps:

pkg_path <- "/home/fabri/test/testHarness/RcppDeepState/inst/testpkgs/testSAN"
fun_name <- "rcpp_write_index_outofbound"

deepstate_fun_create(pkg_path, fun_name)
deepstate_fuzz_fun(pkg_path, fun_name)
deepstate_analyze_fun(pkg_path, fun_name)

The issue is that, before I implemented the modifications in this pull request, the R CMD install command is never executed to produce the shared object: R CMD install is only run when the user invokes the deepstate_pkg_create function, or in other words when the user tests the full package. As a result, the shared object is created only when the user tests the full package.

I don't understand why the shared object/so file would ever be absent, if the package was installed?

The package is installed using R CMD install, but only if the user first executes deepstate_pkg_create, or else the shared object files are missing. The shared object is not generated in the preceding example because deepstate_pkg_create is never called in the sequence of function calls. The solution is to include a second check in the makefile generating procedure to check that the shared object file is there. As a result, even if the user does not explicitly execute deepstate_pkg_create, the shared object will still be generated due to this second check.

tdhock · 2022-08-31T18:16:00Z

ok, thanks for the detailed explanation. I think I understand now. Basically, the user may be trying to compile a test harness in a source package which has not yet been installed on the system. The confusing part for me was that R CMD INSTALL is called in this case not actually for its main purpose of installing to the system library, but for its side effect of compiling the cpp files in the src directory, and creating a so file in there. Right?

FabrizioSandri · 2022-08-31T18:25:44Z

The confusing part for me was that R CMD INSTALL is called in this case not actually for its main purpose of installing to the system library, but for its side effect of compiling the cpp files in the src directory, and creating a so file in there. Right?

Yes, this is correct. Now I'm attempting to figure out why this pull request's CI is failing. It appears to be due to the following error message caused by the function system("R CMD INSTALL", ...)

  'R' should not be used without a path -- see par. 1.6 of the manual

I tried running the tests locally and they were successful, but using r-lib/actions causes CI to fail with this issue.

tdhock · 2022-08-31T22:48:25Z

I believe this refers to the following from https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Writing-portable-packages

Do not invoke R by plain R, Rscript or (on Windows) Rterm in your examples, tests, vignettes, makefiles or other scripts. As pointed out in several places earlier in this manual, use something like

"$(R_HOME)/bin/Rscript"
"$(R_HOME)/bin$(R_ARCH_BIN)/Rterm"

with appropriate quotes (as, although not recommended, R_HOME can contain spaces).

tdhock · 2022-08-31T22:49:28Z

rather than using system("R CMD INSTALL path/to/pkg") I wonder if you could use install.packages("path/to/pkg", repo=NULL) ? or devtools::install("path/to/pkg") ?

FabrizioSandri · 2022-09-01T08:54:47Z

You are correct! Thank you for your help; this resolved the issue. I changed the system("R CMD INSTALL path/to/pkg") call to install.packages("path/to/pkg", repo=NULL). I also deleted the previously mentioned system("R CMD INSTALL path/to/pkg") call from the deepstate_pkg_create function: shared objects are already checked for existence inside the makefile generation procedure, and if they are not present, they are generated. In this manner, each sequence of function calls will cause the shared object file to be generated.

Regarding the use of devtools::install("path/to/pkg"), I believe it is the user's duty to ensure that all of the dependencies of the package are installed.
In reality, I introduced another step within the makefile creation procedure that checks whether the shared object was not created after running install.packages(...); if it was not created, it indicates that a dependency is missing and the user will be notified with an error.

if (length(Sys.glob(shared_objects)) <= 0) {
  error_msg <- paste("ERROR: the shared object for your package cannot be",
                     "generated. This is probably caused by a missing",
                     "dependency. Please install all the dependencies for",
                     "your package.")
  stop(error_msg)
}

As discussed during the conversation we had on Tuesday I will ensure that RcppDeepState-action installs the missing package dependencies running devtools::install("path/to/pkg") before running RcppDeepState. I have implemented this in the following pull request

As we mentioned on Tuesday, I will make sure that RcppDeepState-action installs any missing package dependencies by executing devtools::install("path/to/pkg") before launching RcppDeepState. This is what I've done in the following pull request:

Dependencies issue RcppDeepState-action#20

tdhock · 2022-09-01T16:23:03Z

R/fun_makefile_create.R

+    install.packages(package, repo=NULL)
+
+    if (length(Sys.glob(shared_objects)) <= 0) {
+      error_msg <- paste("ERROR: the shared object for your package cannot be",


you don't need a paste and intermediate error_msg variable here, instead just use

stop( "the shared object ", "for your")

also please remove ERROR: since R adds that automatically, to avoid displaying Error: ERROR: etc.

Fixed with commits f957075 and f739ebc

tdhock · 2022-09-01T16:24:32Z

R/fun_makefile_create.R

    makevars_file <- file.path(package, "src", "Makevars")
    if (dir.exists(file.path(package, "src"))) {
-      makevars_content <- "PKG_CXXFLAGS += -g \n"
-      write(makevars_content, makevars_file, append=TRUE)
+        makevars_content <- "PKG_CXXFLAGS += -g \n"


is there any reason to have a different/non-standard 4 space indent here? if not can you please change back to 2 space indent, which seems conssitent with the surrounding code.

My mistake, I updated this portion of code with commit f957075

tdhock · 2022-09-01T16:27:08Z

NAMESPACE

+import(utils)
 import(xml2)
 importFrom(utils,unzip)


typically to avoid confusion, you should not use both importFrom(utils,unzip) and import(utils). what other function from utils are you using? Please either add that to the importFrom, or remove the importFrom, keep the import, and add utils:: prefix to the R code from utils, for example utils::unzip

I'm using unzip and install.packages from utils. If I use import for utils, I get the following warning:

Warning: replacing previous import ‘Rcpp::prompt’ by ‘utils::prompt’ when loading ‘pkgA’

There is most likely some conflict between utils and Rcpp. Instead, I replaced import with importFrom with commit 11eaa1b.

tdhock · 2022-09-01T16:35:05Z

R/fun_makefile_create.R

+      stop("ERROR: the shared object for your package cannot be generated.",
+           "This is probably caused by a missing dependency. Please install",
+           "all the dependencies for your package.")


> stop("ERROR: one sentence.", "Another sentence.") Error: ERROR: one sentence.Another sentence.

please remove ERROR: and use a single argument to stop:

stop("One sentence. Another sentence.")

usually it is preferable to have 80 character max lines, and break lines like you do here, but for long character strings used in messages, it is preferable to not break them over multiple lines, to facilitate translation -- translators would have to translate each string separately. see https://github.com/MichaelChirico/potools for more info

Thank you for your detailed explanation; I followed your suggestions and updated this with commit 3ede877.

tdhock · 2022-09-01T17:05:09Z

great!

tdhock · 2022-09-01T17:08:14Z

R/fun_makefile_create.R

+  compiler_cppflags <- paste(cppflags, collapse=" -I ")
+  cppflags_write <- paste0("CPPFLAGS=-I",compiler_cppflags)


Is this logic tested? I thought there was supposed to be no space between -I and the include path?
anyway please remove repetition of -I in this logic. I believe it should be paste(paste0("-I", cppflags), collapse=" ")

Good catch! There should be no space between -I and the include path. I tested this locally and it works, but I'll let CI finish to ensure that everything is working correctly.

Updated with commit 179ac76

tdhock · 2022-09-01T17:09:41Z

R/fun_makefile_create.R

-  compiler_ldflags <- paste0("-L", rinside_lib, " -Wl,-rpath=", rinside_lib) 
-  compiler_ldflags <- paste(compiler_ldflags, "-L${R_LIB} -Wl,-rpath=${R_LIB}")
-  compiler_ldflags <- paste(compiler_ldflags, paste0("-L", deepstate_build, " -Wl,-rpath=", deepstate_build))
-  write_to_file <- paste0(write_to_file, "LDFLAGS=", compiler_ldflags, "\n")
+  ldflag <- function(path) paste0("-L", path, " -Wl,-rpath=", path) 
+  ldflags <- c(ldflag(rinside_lib), ldflag("${R_LIB}"), ldflag(deepstate_build))


this is better! nice cleanup. the intent is much more clear this way.

FabrizioSandri · 2022-09-02T08:13:47Z

@tdhock As mentioned during the meeting, I have published a new blog article on the beta testing results. If you agree, I'll merge this pull request as well as FabrizioSandri/RcppDeepState-action#20.

tdhock · 2022-09-02T14:46:47Z

sounds great please merge!

FabrizioSandri added 2 commits August 30, 2022 20:31

abort if missing shared object

2faa7a8

removed package installation

e45456a

FabrizioSandri mentioned this pull request Aug 30, 2022

Dependencies issue FabrizioSandri/RcppDeepState-action#20

Merged

try to generate the shared object before stopping

f861ff5

check if shared object exists

4a335f4

FabrizioSandri added 3 commits September 1, 2022 09:05

roxygen documentation

383ddb7

use install.packages rather than R CMD INSTALL

0fb07e8

utils::install.packages namespace

4eeb0b4

code refactoring

f3de41e

tdhock reviewed Sep 1, 2022

View reviewed changes

FabrizioSandri added 2 commits September 1, 2022 18:29

code refactoring 2

f957075

unnecessary stop "ERROR" message

f739ebc

tdhock reviewed Sep 1, 2022

View reviewed changes

FabrizioSandri added 2 commits September 1, 2022 18:51

avoid splitting long strings on multiple lines

3ede877

importFrom namespace fix

11eaa1b

tdhock reviewed Sep 1, 2022

View reviewed changes

FabrizioSandri added 2 commits September 1, 2022 19:16

cppflags spacing fix

a6f1375

cppflags spacing fix

179ac76

accidental commit of the wrong file

e778498

FabrizioSandri merged commit 5767d29 into master Sep 2, 2022

FabrizioSandri mentioned this pull request Sep 2, 2022

Not finding source files from DESCRIPTION LinkingTo: field? FabrizioSandri/RcppDeepState-action#19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dependencies issue #27

Dependencies issue #27

FabrizioSandri commented Aug 30, 2022

FabrizioSandri commented Aug 30, 2022

tdhock commented Aug 30, 2022

FabrizioSandri commented Aug 31, 2022

tdhock commented Aug 31, 2022

FabrizioSandri commented Aug 31, 2022 •

edited

tdhock commented Aug 31, 2022

tdhock commented Aug 31, 2022

FabrizioSandri commented Sep 1, 2022 •

edited

tdhock Sep 1, 2022

FabrizioSandri Sep 1, 2022

tdhock Sep 1, 2022

FabrizioSandri Sep 1, 2022

tdhock Sep 1, 2022

FabrizioSandri Sep 1, 2022

tdhock Sep 1, 2022

FabrizioSandri Sep 1, 2022

tdhock commented Sep 1, 2022

tdhock Sep 1, 2022 •

edited

FabrizioSandri Sep 1, 2022 •

edited

tdhock Sep 1, 2022

FabrizioSandri Sep 1, 2022

FabrizioSandri commented Sep 2, 2022 •

edited

tdhock commented Sep 2, 2022

		compiler_cppflags <- paste(cppflags, collapse=" -I ")
		cppflags_write <- paste0("CPPFLAGS=-I",compiler_cppflags)

Dependencies issue #27

Dependencies issue #27

Conversation

FabrizioSandri commented Aug 30, 2022

FabrizioSandri commented Aug 30, 2022

Proof of concept

tdhock commented Aug 30, 2022

FabrizioSandri commented Aug 31, 2022

tdhock commented Aug 31, 2022

FabrizioSandri commented Aug 31, 2022 • edited

tdhock commented Aug 31, 2022

tdhock commented Aug 31, 2022

FabrizioSandri commented Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdhock commented Sep 1, 2022

tdhock Sep 1, 2022 • edited

Choose a reason for hiding this comment

FabrizioSandri Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabrizioSandri commented Sep 2, 2022 • edited

tdhock commented Sep 2, 2022

FabrizioSandri commented Aug 31, 2022 •

edited

FabrizioSandri commented Sep 1, 2022 •

edited

tdhock Sep 1, 2022 •

edited

FabrizioSandri Sep 1, 2022 •

edited

FabrizioSandri commented Sep 2, 2022 •

edited