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

use pkg dir rather that getwd() for native routine registration #697

Merged
merged 1 commit into from May 23, 2017

Conversation

Projects
None yet
5 participants
@jjallaire
Member

jjallaire commented May 23, 2017

Execute tools::package_native_routine_registration_skeleton within package rather than current working directory.

Typically compileAttributes is called with the package directory as the current working directory. However, it can be invoked via e.g. compileAttributes(pkgdir = "foo"). In that case we were calling tools::package_native_routine_registration_skeleton in the current working directory rather than in the specified pkgdir. This would result in missing registrations (or over-registrations if the current directory happened to be another package).

The bug should not affect most normal uses of compileAttributes so there isn't a need to do a patch release for this fix.

Execute tools::package_native_routine_registration_skeleton within pa…
…ckage rather than current working directory

@jjallaire jjallaire changed the title from use pkg dir rather that `getwd()` for native routine registration to use pkg dir rather that getwd() for native routine registration May 23, 2017

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire May 23, 2017

Member

As it turns out this does fix the devtools problem after all.

To further clarify, there will be a potential problem if you invoke compileAttributes on a package within the working directory of another package that includes native routines.

So with the devtools test, it was probably invoking something like this with devtools as the working directory:

devtools::install("/tmp/test-pkg-dir")

Wherein compileAttributes was called under the hood by devtools.

So as a practical matter, this will affect out-of package source directory use of devtools package building functions. Note that this is all at development time and any error will be immediately manifest and correctable by installing the development version of Rcpp (just reinforcing that I think no patch release to CRAN is required).

Member

jjallaire commented May 23, 2017

As it turns out this does fix the devtools problem after all.

To further clarify, there will be a potential problem if you invoke compileAttributes on a package within the working directory of another package that includes native routines.

So with the devtools test, it was probably invoking something like this with devtools as the working directory:

devtools::install("/tmp/test-pkg-dir")

Wherein compileAttributes was called under the hood by devtools.

So as a practical matter, this will affect out-of package source directory use of devtools package building functions. Note that this is all at development time and any error will be immediately manifest and correctable by installing the development version of Rcpp (just reinforcing that I think no patch release to CRAN is required).

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 23, 2017

Codecov Report

Merging #697 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #697      +/-   ##
==========================================
+ Coverage   89.51%   89.54%   +0.03%     
==========================================
  Files          66       66              
  Lines        3586     3587       +1     
==========================================
+ Hits         3210     3212       +2     
+ Misses        376      375       -1
Impacted Files Coverage Δ
src/attributes.cpp 98.4% <ø> (ø) ⬆️
R/Attributes.R 97.61% <100%> (ø) ⬆️
R/loadModule.R 100% <0%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48b13bc...76090d3. Read the comment docs.

codecov-io commented May 23, 2017

Codecov Report

Merging #697 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #697      +/-   ##
==========================================
+ Coverage   89.51%   89.54%   +0.03%     
==========================================
  Files          66       66              
  Lines        3586     3587       +1     
==========================================
+ Hits         3210     3212       +2     
+ Misses        376      375       -1
Impacted Files Coverage Δ
src/attributes.cpp 98.4% <ø> (ø) ⬆️
R/Attributes.R 97.61% <100%> (ø) ⬆️
R/loadModule.R 100% <0%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48b13bc...76090d3. Read the comment docs.

@jjallaire jjallaire merged commit 66612ae into master May 23, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jimhester

This comment has been minimized.

Show comment
Hide comment
@jimhester

jimhester May 23, 2017

Contributor

CRAN wants us to fix the failing devtools test caused by the change in behavior.

So either there needs to be a devtools release that works around this or an Rcpp release with the fix from this PR.

We would prefer the latter, devtools::install('xyz') is pretty common devtools usage.

Contributor

jimhester commented May 23, 2017

CRAN wants us to fix the failing devtools test caused by the change in behavior.

So either there needs to be a devtools release that works around this or an Rcpp release with the fix from this PR.

We would prefer the latter, devtools::install('xyz') is pretty common devtools usage.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel May 23, 2017

Member

Feel free to point them to the pull request and tell them to help themselves.

In other words I don't mind them camouflaging 0.12.11.1 as 0.12.11 but I do not plan to make a point release. I would go as far as putting 0.12.11.1 onto the rcpp-drat repo. They can install from there...

Or just make a devtools point release.

Member

eddelbuettel commented May 23, 2017

Feel free to point them to the pull request and tell them to help themselves.

In other words I don't mind them camouflaging 0.12.11.1 as 0.12.11 but I do not plan to make a point release. I would go as far as putting 0.12.11.1 onto the rcpp-drat repo. They can install from there...

Or just make a devtools point release.

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire May 23, 2017

Member

Rcpp releases are huge affairs (16 hours of revdep checking, CRAN rebuilding a huge number of binaries, etc.). The devtools workaround would just be to setwd to the pkg dir before calling compileAttributes, something like this:

withr_with_dir(pkg$path, {
   Rcpp::compileAttributes()
})
Member

jjallaire commented May 23, 2017

Rcpp releases are huge affairs (16 hours of revdep checking, CRAN rebuilding a huge number of binaries, etc.). The devtools workaround would just be to setwd to the pkg dir before calling compileAttributes, something like this:

withr_with_dir(pkg$path, {
   Rcpp::compileAttributes()
})
@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley May 23, 2017

Contributor

Is there a reason this R CMD check failure wasn't reported to us during the initial round of release checks?

Contributor

hadley commented May 23, 2017

Is there a reason this R CMD check failure wasn't reported to us during the initial round of release checks?

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire May 23, 2017

Member
Member

jjallaire commented May 23, 2017

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel May 23, 2017

Member

Is there a reason the devtools team didn't test the release candidate?

Member

eddelbuettel commented May 23, 2017

Is there a reason the devtools team didn't test the release candidate?

@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley May 23, 2017

Contributor

Because we were never notified of it?

But this thread clearly isn't going anywhere, so I don't see any point in continuing the conversation. We'll rush a devtools fix for CRAN.

Contributor

hadley commented May 23, 2017

Because we were never notified of it?

But this thread clearly isn't going anywhere, so I don't see any point in continuing the conversation. We'll rush a devtools fix for CRAN.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel May 23, 2017

Member

If you depend on a tool, maybe you should follow it?

This is all spillover from the R 3.4.0 release and their somewhat poorly communicated decisions / changes and their impact on us. We made the package registration code change late, and it was risqu'e to release. So you got burned. Such is life.

I don't use devtools. I believe a few folks within Rcpp Core do. Still nobody noticed. Not ideal.

And again: "Suggests != Depends". My life is too short too, so I cannot test Suggests: or recursive Depends:. One day when the R Consortium builds a more powerful test battery we may get to issues like this. Right now we don't.

Member

eddelbuettel commented May 23, 2017

If you depend on a tool, maybe you should follow it?

This is all spillover from the R 3.4.0 release and their somewhat poorly communicated decisions / changes and their impact on us. We made the package registration code change late, and it was risqu'e to release. So you got burned. Such is life.

I don't use devtools. I believe a few folks within Rcpp Core do. Still nobody noticed. Not ideal.

And again: "Suggests != Depends". My life is too short too, so I cannot test Suggests: or recursive Depends:. One day when the R Consortium builds a more powerful test battery we may get to issues like this. Right now we don't.

@eddelbuettel eddelbuettel deleted the bugfix/pkg-dir-routine-registration branch May 23, 2017

jimhester added a commit to r-lib/devtools that referenced this pull request Jun 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment