From 89ce869a33432657058894ac3e27a9d0a95299a0 Mon Sep 17 00:00:00 2001 From: Jim Hester Date: Wed, 30 Jun 2021 16:52:03 -0400 Subject: [PATCH 1/7] Rough implementation to use the normal path names in the error outputs --- R/source.R | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/R/source.R b/R/source.R index 3fefeb44..b6f4db86 100644 --- a/R/source.R +++ b/R/source.R @@ -96,11 +96,14 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu package <- tools::file_path_sans_ext(generate_cpp_name(file)) } - # file not points to another location - file.copy(file, file.path(dir, "src", name)) - #change variable name to reflect this - file <- file.path(dir, "src", name) + orig_path <- normalizePath(dirname(file)) + new_path <- normalizePath(file.path(dir, "src")) + + # file now points to another location + file.copy(file, file.path(new_path, name)) + #change variable name to reflect this + file <- file.path(new_path, name) suppressWarnings( all_decorations <- decor::cpp_decorations(dir, is_attribute = TRUE) @@ -128,12 +131,16 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu makevars_content <- generate_makevars(includes, cxx_std) - brio::write_lines(makevars_content, file.path(dir, "src", "Makevars")) + brio::write_lines(makevars_content, file.path(new_path, "Makevars")) source_files <- normalizePath(c(file, cpp_path), winslash = "/") - res <- callr::rcmd("SHLIB", source_files, user_profile = TRUE, show = !quiet, wd = file.path(dir, "src")) + res <- callr::rcmd("SHLIB", source_files, user_profile = TRUE, show = !quiet, wd = new_path) if (res$status != 0) { - cat(res$stderr) + error_messages <- res$stderr + + # Substitue temporary file path with original file path + error_messages <- gsub(file.path(new_path, basename(file)), file.path(orig_path, basename(file)), error_messages, fixed = TRUE) + cat(error_messages) stop("Compilation failed.", call. = FALSE) } From c3323297939e3464a61a581e39d39fa95c0e4333 Mon Sep 17 00:00:00 2001 From: sbearrows Date: Tue, 6 Jul 2021 10:54:51 -0600 Subject: [PATCH 2/7] tests for error mesg with file path --- R/register.R | 3 +-- R/source.R | 4 ++-- tests/testthat/test-source.R | 9 ++++++++- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/R/register.R b/R/register.R index f0056af9..78fafc2d 100644 --- a/R/register.R +++ b/R/register.R @@ -277,13 +277,12 @@ get_cpp_register_needs <- function() { strsplit(res, "[[:space:]]*,[[:space:]]*")[[1]] } -check_valid_attributes <- function(decorations) { +check_valid_attributes <- function(decorations, file = decorations$file) { bad_decor <- !decorations$decoration %in% c("cpp11::register", "cpp11::init") if(any(bad_decor)) { lines <- decorations$line[bad_decor] - file <- decorations$file[bad_decor] names <- decorations$decoration[bad_decor] bad_lines <- glue::glue_collapse(glue::glue("- Invalid attribute `{names}` on line {lines} in file '{file}'."), "\n") diff --git a/R/source.R b/R/source.R index b6f4db86..bd861dab 100644 --- a/R/source.R +++ b/R/source.R @@ -98,7 +98,7 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu orig_path <- normalizePath(dirname(file)) new_path <- normalizePath(file.path(dir, "src")) - + # file now points to another location file.copy(file, file.path(new_path, name)) @@ -109,7 +109,7 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu all_decorations <- decor::cpp_decorations(dir, is_attribute = TRUE) ) - check_valid_attributes(all_decorations) + check_valid_attributes(all_decorations, file = file.path(orig_path, basename(file))) cli_suppress( funs <- get_registered_functions(all_decorations, "cpp11::register") diff --git a/tests/testthat/test-source.R b/tests/testthat/test-source.R index 8d708be3..6a9bfb74 100644 --- a/tests/testthat/test-source.R +++ b/tests/testthat/test-source.R @@ -54,6 +54,13 @@ test_that("cpp_source works with files called `cpp11.cpp`", { expect_true(always_true()) }) +test_that("cpp_source returns original file name on error", { + + expect_error(cpp_source(test_path("single_incorrect.cpp"), clean = TRUE, quiet = TRUE), + normalizePath(test_path("single_incorrect.cpp"))) + +}) + test_that("cpp_source lets you set the C++ standard", { skip_on_os("solaris") skip_on_os("windows") # Older windows toolchains do not support C++14 @@ -152,4 +159,4 @@ test_that("check_valid_attributes returns an error if one or more registers is i x.push_back({"foo"_nm = 1}); return x; }')) -}) \ No newline at end of file +}) From 87c318ede8efad5e9b33cb94bccbcf90b1e6dfaa Mon Sep 17 00:00:00 2001 From: sbearrows Date: Tue, 6 Jul 2021 12:50:58 -0600 Subject: [PATCH 3/7] support windows --- tests/testthat/single_error.cpp | 1 + tests/testthat/test-source.R | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 tests/testthat/single_error.cpp diff --git a/tests/testthat/single_error.cpp b/tests/testthat/single_error.cpp new file mode 100644 index 00000000..8f9b8963 --- /dev/null +++ b/tests/testthat/single_error.cpp @@ -0,0 +1 @@ +[[cpp11::register]] int foo() { return 1 } diff --git a/tests/testthat/test-source.R b/tests/testthat/test-source.R index 6a9bfb74..04ca20bf 100644 --- a/tests/testthat/test-source.R +++ b/tests/testthat/test-source.R @@ -56,8 +56,12 @@ test_that("cpp_source works with files called `cpp11.cpp`", { test_that("cpp_source returns original file name on error", { - expect_error(cpp_source(test_path("single_incorrect.cpp"), clean = TRUE, quiet = TRUE), - normalizePath(test_path("single_incorrect.cpp"))) + expect_output(try(cpp_source(test_path("single_error.cpp")), silent = TRUE), + normalizePath(test_path("single_error.cpp")), fixed = TRUE) + + #error generate for attributes is separate from compilation errors + expect_error(cpp_source(test_path("single_incorrect.cpp")), + normalizePath(test_path("single_incorrect.cpp")), fixed = TRUE) }) From 70ff38401222a15c5a8be5bfbb1d189e51216b67 Mon Sep 17 00:00:00 2001 From: Jim Hester Date: Thu, 8 Jul 2021 16:00:10 -0400 Subject: [PATCH 4/7] Fix compilation on windows --- R/source.R | 4 ++-- tests/testthat/test-source.R | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/R/source.R b/R/source.R index b9d4c2d9..ea698923 100644 --- a/R/source.R +++ b/R/source.R @@ -96,8 +96,8 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu package <- tools::file_path_sans_ext(generate_cpp_name(file)) } - orig_path <- normalizePath(dirname(file)) - new_path <- normalizePath(file.path(dir, "src")) + orig_path <- normalizePath(dirname(file), winslash = "/") + new_path <- normalizePath(file.path(dir, "src"), winslash = "/") # file now points to another location file.copy(file, file.path(new_path, name)) diff --git a/tests/testthat/test-source.R b/tests/testthat/test-source.R index ebe8c5b7..fbf1dc57 100644 --- a/tests/testthat/test-source.R +++ b/tests/testthat/test-source.R @@ -57,11 +57,11 @@ test_that("cpp_source works with files called `cpp11.cpp`", { test_that("cpp_source returns original file name on error", { expect_output(try(cpp_source(test_path("single_error.cpp")), silent = TRUE), - normalizePath(test_path("single_error.cpp")), fixed = TRUE) + normalizePath(test_path("single_error.cpp"), winslash = "/"), fixed = TRUE) #error generate for attributes is separate from compilation errors expect_error(cpp_source(test_path("single_incorrect.cpp")), - normalizePath(test_path("single_incorrect.cpp")), fixed = TRUE) + normalizePath(test_path("single_incorrect.cpp"), winslash = "/"), fixed = TRUE) }) From 41143ff1d2d4202f96291b3b808706cb12eaf487 Mon Sep 17 00:00:00 2001 From: sbearrows Date: Mon, 12 Jul 2021 14:37:19 -0600 Subject: [PATCH 5/7] revert to old names but keep new error messages --- NEWS.md | 2 ++ R/source.R | 17 ++++------------- tests/testthat/test-source.R | 7 +++++++ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/NEWS.md b/NEWS.md index d8fca332..28f355c0 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,8 @@ # cpp11 (development version) * Removed redundant `.Call calls` in cpp11.cpp file (@sbearrows, #170) * Allow cpp11 decorators of the form `cpp11::linking_to` (@sbearrows, #193) +* Error messages now output original file name rather than the temporary file name (@sbearrows, #194) +* Fixed bug when running `cpp_source()` on the same file more than once (@sbearrows, #202) # cpp11 0.3.1 diff --git a/R/source.R b/R/source.R index b9d4c2d9..2bd40d03 100644 --- a/R/source.R +++ b/R/source.R @@ -84,17 +84,8 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu stop("`file` must have a `.cpp` or `.cc` extension") } - base_cpp <- (basename(file) %in% "cpp11.cpp") - - if (base_cpp) { - name <- set_cpp_name(file) - package <- tools::file_path_sans_ext(name) - } - else { - # name and package might be different if cpp_source was called multiple times - name <- basename(file) - package <- tools::file_path_sans_ext(generate_cpp_name(file)) - } + name <- generate_cpp_name(file) + package <- tools::file_path_sans_ext(name) orig_path <- normalizePath(dirname(file)) new_path <- normalizePath(file.path(dir, "src")) @@ -109,6 +100,7 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu all_decorations <- decor::cpp_decorations(dir, is_attribute = TRUE) ) + #provide original path for error messages check_valid_attributes(all_decorations, file = file.path(orig_path, basename(file))) cli_suppress( @@ -138,13 +130,12 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu if (res$status != 0) { error_messages <- res$stderr - # Substitue temporary file path with original file path + # Substitute temporary file path with original file path error_messages <- gsub(file.path(new_path, basename(file)), file.path(orig_path, basename(file)), error_messages, fixed = TRUE) cat(error_messages) stop("Compilation failed.", call. = FALSE) } - shared_lib <- file.path(dir, "src", paste0(tools::file_path_sans_ext(basename(file)), .Platform$dynlib.ext)) r_path <- file.path(dir, "R", "cpp11.R") brio::write_lines(r_functions, r_path) diff --git a/tests/testthat/test-source.R b/tests/testthat/test-source.R index ebe8c5b7..1d51110f 100644 --- a/tests/testthat/test-source.R +++ b/tests/testthat/test-source.R @@ -212,3 +212,10 @@ test_that("check_valid_attributes returns an error if one or more registers is i ) }) + +test_that("cpp_source(d) functions work after sourcing file more than once", { + cpp11::cpp_source(test_path("single.cpp")) + expect_equal(foo(), 1) + cpp11::cpp_source(test_path("single.cpp")) + expect_equal(foo(), 1) +}) From c39a4e1360c8b7b3d6926addae280a7eb7db20d5 Mon Sep 17 00:00:00 2001 From: sbearrows Date: Wed, 14 Jul 2021 11:27:56 -0600 Subject: [PATCH 6/7] incorporate Jims edits --- R/source.R | 24 ++++++++++-------------- tests/testthat/test-source.R | 18 ++++++++---------- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/R/source.R b/R/source.R index b2d14819..e9b4f173 100644 --- a/R/source.R +++ b/R/source.R @@ -94,21 +94,25 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu file.copy(file, file.path(new_path, name)) #change variable name to reflect this - file <- file.path(new_path, name) + new_file_path <- file.path(new_path, name) + new_file_name <- basename(new_file_path) + + new_file <- file.path(new_path, new_file_name) + orig_file <- file.path(orig_path, new_file_name) suppressWarnings( all_decorations <- decor::cpp_decorations(dir, is_attribute = TRUE) ) #provide original path for error messages - check_valid_attributes(all_decorations, file = file.path(orig_path, basename(file))) + check_valid_attributes(all_decorations, file = orig_file) cli_suppress( funs <- get_registered_functions(all_decorations, "cpp11::register") ) cpp_functions_definitions <- generate_cpp_functions(funs, package = package) - cpp_path <- file.path(dirname(file), "cpp11.cpp") + cpp_path <- file.path(dirname(new_file_path), "cpp11.cpp") brio::write_lines(c('#include "cpp11/declarations.hpp"', "using namespace cpp11;", cpp_functions_definitions), cpp_path) linking_to <- union(get_linking_to(all_decorations), "cpp11") @@ -125,18 +129,18 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu brio::write_lines(makevars_content, file.path(new_path, "Makevars")) - source_files <- normalizePath(c(file, cpp_path), winslash = "/") + source_files <- normalizePath(c(new_file_path, cpp_path), winslash = "/") res <- callr::rcmd("SHLIB", source_files, user_profile = TRUE, show = !quiet, wd = new_path) if (res$status != 0) { error_messages <- res$stderr # Substitute temporary file path with original file path - error_messages <- gsub(file.path(new_path, basename(file)), file.path(orig_path, basename(file)), error_messages, fixed = TRUE) + error_messages <- gsub(tools::file_path_sans_ext(new_file), tools::file_path_sans_ext(orig_file), res$stderr, fixed = TRUE) cat(error_messages) stop("Compilation failed.", call. = FALSE) } - shared_lib <- file.path(dir, "src", paste0(tools::file_path_sans_ext(basename(file)), .Platform$dynlib.ext)) + shared_lib <- file.path(dir, "src", paste0(tools::file_path_sans_ext(new_file_name), .Platform$dynlib.ext)) r_path <- file.path(dir, "R", "cpp11.R") brio::write_lines(r_functions, r_path) source(r_path, local = env) @@ -147,14 +151,6 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu the <- new.env(parent = emptyenv()) the$count <- 0L -set_cpp_name <- function(name) { - ext <- tools::file_ext(name) - root <- tools::file_path_sans_ext(basename(name)) - count <- 2 - new_name <- sprintf("%s_%i", root, count) - sprintf("%s.%s", new_name, ext) -} - generate_cpp_name <- function(name, loaded_dlls = c("cpp11", names(getLoadedDLLs()))) { ext <- tools::file_ext(name) root <- tools::file_path_sans_ext(basename(name)) diff --git a/tests/testthat/test-source.R b/tests/testthat/test-source.R index 3c3ed354..8f813d0e 100644 --- a/tests/testthat/test-source.R +++ b/tests/testthat/test-source.R @@ -56,11 +56,11 @@ test_that("cpp_source works with files called `cpp11.cpp`", { test_that("cpp_source returns original file name on error", { - expect_output(try(cpp_source(test_path("single_error.cpp")), silent = TRUE), + expect_output(try(cpp_source(test_path("single_error.cpp"), clean = TRUE), silent = TRUE), normalizePath(test_path("single_error.cpp"), winslash = "/"), fixed = TRUE) - #error generate for attributes is separate from compilation errors - expect_error(cpp_source(test_path("single_incorrect.cpp")), + #error generated for incorrect attributes is separate from compilation errors + expect_error(cpp_source(test_path("single_incorrect.cpp"), clean = TRUE), normalizePath(test_path("single_incorrect.cpp"), winslash = "/"), fixed = TRUE) }) @@ -115,7 +115,7 @@ test_that("generate_include_paths handles paths with spaces", { test_that("check_valid_attributes does not return an error if all registers are correct", { expect_error_free( - cpp11::cpp_source(code = '#include + cpp11::cpp_source(clean = TRUE, code = '#include using namespace cpp11::literals; [[cpp11::register]] cpp11::list fn() { @@ -130,7 +130,7 @@ test_that("check_valid_attributes does not return an error if all registers are return x; }')) expect_error_free( - cpp11::cpp_source( + cpp11::cpp_source(clean = TRUE, code = '#include #include @@ -208,14 +208,12 @@ test_that("check_valid_attributes returns an error if one or more registers is i pb.tick(); } } -') - ) - +')) }) test_that("cpp_source(d) functions work after sourcing file more than once", { - cpp11::cpp_source(test_path("single.cpp")) + cpp11::cpp_source(test_path("single.cpp"), clean = TRUE) expect_equal(foo(), 1) - cpp11::cpp_source(test_path("single.cpp")) + cpp11::cpp_source(test_path("single.cpp"), clean = TRUE) expect_equal(foo(), 1) }) From 52a632604ae0b29d5786e91cebe299ed137d68db Mon Sep 17 00:00:00 2001 From: sbearrows Date: Wed, 14 Jul 2021 11:36:32 -0600 Subject: [PATCH 7/7] tweak file path variable names --- R/source.R | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/R/source.R b/R/source.R index e9b4f173..c868b328 100644 --- a/R/source.R +++ b/R/source.R @@ -87,25 +87,24 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu name <- generate_cpp_name(file) package <- tools::file_path_sans_ext(name) - orig_path <- normalizePath(dirname(file), winslash = "/") - new_path <- normalizePath(file.path(dir, "src"), winslash = "/") + orig_dir <- normalizePath(dirname(file), winslash = "/") + new_dir <- normalizePath(file.path(dir, "src"), winslash = "/") # file now points to another location - file.copy(file, file.path(new_path, name)) + file.copy(file, file.path(new_dir, name)) #change variable name to reflect this - new_file_path <- file.path(new_path, name) + new_file_path <- file.path(new_dir, name) new_file_name <- basename(new_file_path) - new_file <- file.path(new_path, new_file_name) - orig_file <- file.path(orig_path, new_file_name) + orig_file_path <- file.path(orig_dir, new_file_name) suppressWarnings( all_decorations <- decor::cpp_decorations(dir, is_attribute = TRUE) ) #provide original path for error messages - check_valid_attributes(all_decorations, file = orig_file) + check_valid_attributes(all_decorations, file = orig_file_path) cli_suppress( funs <- get_registered_functions(all_decorations, "cpp11::register") @@ -127,15 +126,15 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu makevars_content <- generate_makevars(includes, cxx_std) - brio::write_lines(makevars_content, file.path(new_path, "Makevars")) + brio::write_lines(makevars_content, file.path(new_dir, "Makevars")) source_files <- normalizePath(c(new_file_path, cpp_path), winslash = "/") - res <- callr::rcmd("SHLIB", source_files, user_profile = TRUE, show = !quiet, wd = new_path) + res <- callr::rcmd("SHLIB", source_files, user_profile = TRUE, show = !quiet, wd = new_dir) if (res$status != 0) { error_messages <- res$stderr # Substitute temporary file path with original file path - error_messages <- gsub(tools::file_path_sans_ext(new_file), tools::file_path_sans_ext(orig_file), res$stderr, fixed = TRUE) + error_messages <- gsub(tools::file_path_sans_ext(new_file_path), tools::file_path_sans_ext(orig_file_path), error_messages, fixed = TRUE) cat(error_messages) stop("Compilation failed.", call. = FALSE) }