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

cpp11 dynlib #1921

Merged
merged 7 commits into from
Nov 14, 2023
Merged

cpp11 dynlib #1921

merged 7 commits into from
Nov 14, 2023

Conversation

pachadotdev
Copy link
Contributor

Hi @hadley

These minimal changes allow the user to just write functions of the form

#include <cpp11.hpp>

using namespace cpp11;

[[cpp11::register]] int hello(int x) {
    return x + 1;
}

Immediately after using use_cpp11().

With this change the user can run devtools::load_all() and hello(1) will work. Otherwise it says pkgname_hello not found.

Semi-related to r-lib/cpp11#341

@hadley
Copy link
Member

hadley commented Nov 11, 2023

I think it would be better to follow a similar process to use_import_from(), i.e. use roxygen_ns_append() to write to the package doc, and then run roxygen_update_ns() to automatically update the namespace and reload the package.

@pachadotdev
Copy link
Contributor Author

I think it would be better to follow a similar process to use_import_from(), i.e. use roxygen_ns_append() to write to the package doc, and then run roxygen_update_ns() to automatically update the namespace and reload the package.

ready!
good suggestion, I just tried with usethis::create_package() and it works

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also add a news bullet and a test? (Probably something in "use_cpp11() creates files/dirs, edits DESCRIPTION and .gitignore")

R/cpp11.R Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@pachadotdev
Copy link
Contributor Author

@hadley it should be ok now

@hadley hadley merged commit 9ac020d into r-lib:main Nov 14, 2023
2 of 13 checks passed
@hadley
Copy link
Member

hadley commented Nov 14, 2023

Thanks!

@jennybc
Copy link
Member

jennybc commented Feb 13, 2024

I have reverted this because I need to release usethis under a CRAN deadline and CI has been failing ever since this PR was merged. This change creates a test-time dependency on cpp11 and decor, neither of which usethis has a direct or indirect dependency on. I've fiddled around with the failing tests a bit, but there's no easy way, e.g. via a simple mock, to patch this up quickly, because the dependencies are needed for the roxygenize() step which is precisely the point. So @pachadotdev or @hadley if you care a lot about this, we need to either give up on testing the feature or invest time in more refactoring and mocking.

── Error ('test-cpp11.R:19:3'): use_cpp11() creates files/dirs, edits DESCRIPTION and .gitignore ──
<packageNotFoundError/error/condition>
Error in `loadNamespace(x)`: there is no package called 'cpp11'
Backtrace:
     ▆
  1. ├─usethis::use_cpp11() at test-cpp11.R:19:3
  2. │ └─usethis:::roxygen_update_ns() at usethis/R/cpp11.R:31:3
  3. │   ├─utils::capture.output(...) at usethis/R/roxygen.R:105:3
  4. │   │ └─base::withVisible(...elt(i))
  5. │   ├─base::suppressMessages(roxygen2::roxygenise(proj_get(), "namespace")) at usethis/R/roxygen.R:105:3
  6. │   │ └─base::withCallingHandlers(...)
  7. │   └─roxygen2::roxygenise(proj_get(), "namespace") at usethis/R/roxygen.R:105:3
  8. │     └─roxygen2 (local) load_code(base_path)
  9. │       └─pkgload::load_all(path, helpers = FALSE, attach_testthat = FALSE)
 10. │         └─pkgbuild::compile_dll(path, quiet = quiet)
 11. │           └─pkgbuild:::update_registration(...)
 12. └─base::loadNamespace(x)
 13.   └─base::withRestarts(stop(cond), retry_loadNamespace = function() NULL)
 14.     └─base (local) withOneRestart(expr, restarts[[1L]])
 15.       └─base (local) doWithOneRestart(return(expr), restart)

@pachadotdev
Copy link
Contributor Author

I have reverted this because I need to release usethis under a CRAN deadline and CI has been failing ever since this PR was merged. This change creates a test-time dependency on cpp11 and decor, neither of which usethis has a direct or indirect dependency on. I've fiddled around with the failing tests a bit, but there's no easy way, e.g. via a simple mock, to patch this up quickly, because the dependencies are needed for the roxygenize() step which is precisely the point. So @pachadotdev or @hadley if you care a lot about this, we need to either give up on testing the feature or invest time in more refactoring and mocking.

── Error ('test-cpp11.R:19:3'): use_cpp11() creates files/dirs, edits DESCRIPTION and .gitignore ──
<packageNotFoundError/error/condition>
Error in `loadNamespace(x)`: there is no package called 'cpp11'
Backtrace:
     ▆
  1. ├─usethis::use_cpp11() at test-cpp11.R:19:3
  2. │ └─usethis:::roxygen_update_ns() at usethis/R/cpp11.R:31:3
  3. │   ├─utils::capture.output(...) at usethis/R/roxygen.R:105:3
  4. │   │ └─base::withVisible(...elt(i))
  5. │   ├─base::suppressMessages(roxygen2::roxygenise(proj_get(), "namespace")) at usethis/R/roxygen.R:105:3
  6. │   │ └─base::withCallingHandlers(...)
  7. │   └─roxygen2::roxygenise(proj_get(), "namespace") at usethis/R/roxygen.R:105:3
  8. │     └─roxygen2 (local) load_code(base_path)
  9. │       └─pkgload::load_all(path, helpers = FALSE, attach_testthat = FALSE)
 10. │         └─pkgbuild::compile_dll(path, quiet = quiet)
 11. │           └─pkgbuild:::update_registration(...)
 12. └─base::loadNamespace(x)
 13.   └─base::withRestarts(stop(cond), retry_loadNamespace = function() NULL)
 14.     └─base (local) withOneRestart(expr, restarts[[1L]])
 15.       └─base (local) doWithOneRestart(return(expr), restart)

thanks for letting me know
i am on medical leave but i still check the phone, when i'm back i can try refactoring

@jennybc
Copy link
Member

jennybc commented Feb 13, 2024

I suspect this should just be roxygen_ns_append(...) && roxygen_remind(), as done elsewhere in the package. Initiating use of cpp11 is a once-per-package event, so it's OK for it to involve a couple of steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants