Skip to content

Readonly file io error#1346

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 25, 2024
Merged

Readonly file io error #1346

merged 3 commits into from
Nov 25, 2024
+5 −1

Conversation

tesujimath
Copy link
Contributor

Fixes#1345

-r--r--r-- 1 guestsi users 126 Nov 25 14:53 hello-cpp-world.cpp it23677> Rscript hello-rcpp-world.R Loading required package: Rcpp Hello CPP World

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Preferably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog
eddelbuettel
eddelbuettel reviewed Nov 25, 2024
View reviewed changes
Copy link
Member

@eddelbuetteleddelbuettel left a comment

Choose a reason for hiding this comment

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

Two quick comments, one small request for change -- looks good generally.

src/attributes.cpp
@@ -3279,7 +3279,7 @@ namespace {

// copy the source file to the build dir
Rcpp::Function filecopy = Rcpp::Environment::base_env()["file.copy"];
filecopy(cppSourcePath_, generatedCppSourcePath(), true);
filecopy(cppSourcePath_, generatedCppSourcePath(), true, Rcpp::_["copy.mode"] = false);
Copy link
Member

Choose a reason for hiding this comment

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

Nice. I'll look some more at that tomorrow over morning coffee -- this ensures we get 0644 irrespective of original permissionss?

Copy link
Member

Choose a reason for hiding this comment

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

Have by now caught up onto help(file.copy) -- this is perfect. Had worried that this code, because it dates back to before we had filesystem semantics from C++17 and all that, would have to do dance to do this portably. But farming out to a call to then underlying R instance we always have is golden, and that function has that option. Very nice.

Copy link
Member

Choose a reason for hiding this comment

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

I generally have a mild dislike of Rcpp::_[...] over the preferred Rcpp::Named("...") but they are equivalent, and the former is used in the file already. So may change that another time....

tesujimath pushed a commit to AgResearch/KGD that referenced this pull request Nov 25, 2024
Initial Nix flake using fork of Rcpp for bugfix
007ce6e
Bug in Rcpp blocked using readonly CPP source from Nix store, fixed in this PR, awaiting merge. RcppCore/Rcpp#1346
eddelbuettel
eddelbuettel approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@eddelbuetteleddelbuettel left a comment

Choose a reason for hiding this comment

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

Looks good, one micro nag notwithstanding, and addresses the issue.

Thanks for putting it up.

@eddelbuetteleddelbuettel merged commit fdd7205 into RcppCore:masterNov 25, 2024
9 checks passed
tesujimath pushed a commit to AgResearch/KGD that referenced this pull request Dec 2, 2024
Source-level packaging using a Nix flake
ea5d7b0
Enables running a defined version of KGD from the Nix store, which required a recent version of Rcpp with this bugfix: RcppCore/Rcpp#1346 For example usage see gbs_prism eri-dev branch.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants
@tesujimath@eddelbuettel
close