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

Readonly file io error #1346

Merged
merged 3 commits into from
Nov 25, 2024
Merged

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

Copy link
Member

@eddelbuettel eddelbuettel 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.

ChangeLog Outdated Show resolved Hide resolved
@@ -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
Bug in Rcpp blocked using readonly CPP source from Nix store,
fixed in this PR, awaiting merge.
RcppCore/Rcpp#1346
Copy link
Member

@eddelbuettel eddelbuettel 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.

@eddelbuettel eddelbuettel merged commit fdd7205 into RcppCore:master Nov 25, 2024
9 checks passed
tesujimath pushed a commit to AgResearch/KGD that referenced this pull request Dec 2, 2024
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 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.

File io error for sourceCpp of a readonly file
2 participants