diff --git a/NEWS.md b/NEWS.md index 2c0b9b38..e895abd2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,8 @@ * added `as_double()` and `as_integer()` method to coerce integers to doubles and doubles to integers to doubles (@sbearrows, #46) * 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) * Removed internal instances of `cpp11::stop()` and replaced with C++ exceptions (@sbearrows, #203) # cpp11 0.3.1 diff --git a/R/register.R b/R/register.R index f36b33a1..37c9eb72 100644 --- a/R/register.R +++ b/R/register.R @@ -297,14 +297,13 @@ 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 <- startsWith(decorations$decoration, "cpp11::") & (!decorations$decoration %in% c("cpp11::register", "cpp11::init", "cpp11::linking_to")) 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 e9eb9c82..c868b328 100644 --- a/R/source.R +++ b/R/source.R @@ -84,36 +84,34 @@ 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") + name <- generate_cpp_name(file) + package <- tools::file_path_sans_ext(name) - 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)) - } + 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_dir, name)) - # 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) + new_file_path <- file.path(new_dir, name) + new_file_name <- basename(new_file_path) + orig_file_path <- file.path(orig_dir, new_file_name) suppressWarnings( all_decorations <- decor::cpp_decorations(dir, is_attribute = TRUE) ) - check_valid_attributes(all_decorations) + #provide original path for error messages + check_valid_attributes(all_decorations, file = orig_file_path) 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") @@ -128,17 +126,20 @@ 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_dir, "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")) + source_files <- normalizePath(c(new_file_path, cpp_path), winslash = "/") + res <- callr::rcmd("SHLIB", source_files, user_profile = TRUE, show = !quiet, wd = new_dir) if (res$status != 0) { - cat(res$stderr) + error_messages <- res$stderr + + # Substitute temporary file path with original file path + 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) } - - 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) @@ -149,14 +150,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/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 68f67141..8f813d0e 100644 --- a/tests/testthat/test-source.R +++ b/tests/testthat/test-source.R @@ -54,6 +54,17 @@ 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_output(try(cpp_source(test_path("single_error.cpp"), clean = TRUE), silent = TRUE), + normalizePath(test_path("single_error.cpp"), winslash = "/"), fixed = TRUE) + + #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) + +}) + 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 @@ -104,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() { @@ -119,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 @@ -181,6 +192,8 @@ test_that("check_valid_attributes returns an error if one or more registers is i return x; }')) + + expect_error( cpp11::cpp_source( code = ' @@ -195,6 +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"), clean = TRUE) + expect_equal(foo(), 1) + cpp11::cpp_source(test_path("single.cpp"), clean = TRUE) + expect_equal(foo(), 1) })