awong234 commented 2 years ago

Issue

Assume:

A folder 'lib' in the project directory is used for local libraries, but is not added to the search path.
data.table is installed to this 'lib' directory.
Use import to load functions from data.table.

Data.table was selected as a candidate because it has no dependencies.

import::from(.from = data.table, .library = 'lib', .all = TRUE) does not load functions.

Expected behavior

import::from(.from = data.table, .library = 'lib', .all = TRUE) should load all of data.table's functions.

Actual behavior

import::from(.from = data.table, .library = 'lib', .all = TRUE) returns an error

> Error in loadNamespace(name): there is no package called 'data.table'

Error Details

Error traceback:

> traceback()
9: stop(cond)
8: doWithOneRestart(return(expr), restart)
7: withOneRestart(expr, restarts[[1L]])
6: withRestarts(stop(cond), retry_loadNamespace = function() NULL)
5: loadNamespace(name)
4: getNamespace(ns)
3: asNamespace(ns)
2: getNamespaceExports(spec$pkg)
1: import::from(.from = data.table, .library = "lib", .all = TRUE)

The antecedents to loadNamespace in the call stack do not allow library locations as arguments; when loadNamespace is called, lib.loc can therefore only be NULL.

Solution

Move the call to getNamespaceExports after loadNamespace, so that the namespace is available at that time.

Reprexes

Failure reprex

# Remove user library so that data.table can't be retrieved from there
dir.create('lib', showWarnings = FALSE, recursive = TRUE)
.libPaths(.Library)
.libPaths()
#> [1] "C:/Program Files/R/R-4.1.0/library"
install.packages('import', lib = 'lib')
#> package 'import' successfully unpacked and MD5 sums checked
#> 
#> The downloaded binary packages are in
#>  C:\Users\AlecW\AppData\Local\Temp\Rtmp0olvTw\downloaded_packages
loadNamespace('import', lib.loc = 'lib')
#> <environment: namespace:import>
install.packages('data.table', lib = 'lib')
#> package 'data.table' successfully unpacked and MD5 sums checked
#> 
#> The downloaded binary packages are in
#>  C:\Users\AlecW\AppData\Local\Temp\Rtmp0olvTw\downloaded_packages
# Expect error, cannot load from lib directory
import::from(.from = data.table, .library = 'lib', .all = TRUE)
#> Error in loadNamespace(name): there is no package called 'data.table'
# Expect normal behavior from call to library
library(data.table, lib.loc = 'lib')
#> Warning: package 'data.table' was built under R version 4.1.2
# Clean up
remove.packages('import', lib = 'lib')
remove.packages('data.table', lib = 'lib')

^{Created on 2021-11-29 by the reprex package (v2.0.1)}

Success reprex

# Remove user library so that data.table can't be retrieved from there
# Does NOT add 'lib' to .libPaths, assuming import can find files from any lib
dir.create('lib', showWarnings = FALSE, recursive = TRUE)
.libPaths(.Library)
.libPaths()
#> [1] "C:/Program Files/R/R-4.1.0/library"
# Setup -- install here to ensure code run in project root
install.packages('here', lib = 'lib')
#> package 'here' successfully unpacked and MD5 sums checked
#> 
#> The downloaded binary packages are in
#>  C:\Users\AlecW\AppData\Local\Temp\RtmpQR615Y\downloaded_packages
install.packages('remotes', lib = 'lib')
#> package 'remotes' successfully unpacked and MD5 sums checked
#> 
#> The downloaded binary packages are in
#>  C:\Users\AlecW\AppData\Local\Temp\RtmpQR615Y\downloaded_packages
library(here, lib.loc = 'lib')
#> Warning: package 'here' was built under R version 4.1.2
#> here() starts at C:/Users/AlecW/Documents/Local_Projects/import_test
library(remotes, lib.loc = 'lib')
#> Warning: package 'remotes' was built under R version 4.1.2
# Install import from fork
install_local('import_1.2.1.tar.gz', lib = 'lib')
#> Running `R CMD build`...
#> * checking for file 'C:\Users\AlecW\AppData\Local\Temp\RtmpQR615Y\remotes536440411fe4\import/DESCRIPTION' ... OK
#> * preparing 'import':
#> * checking DESCRIPTION meta-information ... OK
#> * checking vignette meta-information ... OK
#> * checking for LF line-endings in source and make files and shell scripts
#> * checking for empty or unneeded directories
#> * building 'import_1.2.1.tar.gz'
# Load namespace as loading library is not best practice for {import}.
loadNamespace('import', lib.loc = 'lib')
#> <environment: namespace:import>
# Install data.table to test, good candidate having no dependencies.
install.packages('data.table', lib = 'lib')
#> package 'data.table' successfully unpacked and MD5 sums checked
#> 
#> The downloaded binary packages are in
#>  C:\Users\AlecW\AppData\Local\Temp\RtmpQR615Y\downloaded_packages
# Expect no error
import::from(.from = data.table, .library = 'lib', .all = TRUE)
# Clean up
remove.packages('import', lib = 'lib')
remove.packages('data.table', lib = 'lib')

^{Created on 2021-11-29 by the reprex package (v2.0.1)}

The installation of import_1.2.1 is of course from my fork here.

torfason commented 2 years ago

Thanks for the contribution. I've started a unit check run. On the whole, it seems like a useful enhancement, provided there are no regressions. A few questions:

This does not address a specific issue in the tracker, right?
Would it be possible to include a minimal package in the test directory and a unit test for this use case (could such a package be platform independent)?
What would be an appropriate description in the NEWS.md file, and would you care to add that to the PR?

awong234 commented 2 years ago

Hello @torfason ! Thanks for the review -- I definitely consider this a draft at this point and will address your points.

[x] No I do not believe it addresses an existing issue; I can make an issue now.
[x] A minimal package is a good idea -- I am mostly familiar with building packages in Windows and Linux, so I am not certain of complications that might arise on MacOS, but I'm guessing if I keep it really simple there shouldn't be an issue, I'll give that a go.
[x] Will update the NEWS.md and see what you think!

Thanks!

awong234 commented 2 years ago

I put a test package in the test_import directory, and added a line in test_from that installs the package to a tempdir, and then tries to reference the function inside using import::from. All the tests pass locally on my end, let me know if there's anything I should revise. Thanks!

torfason commented 2 years ago

OK, I see build failures on Ubuntu, which is a bummer. Seems like it might be an intermittent issue with the rcmdcheck package on that platform. We can check again in a few days before doing more.

If the build checks get resolved, I'm leaning towards accepting this into the development edition (which is on the master branch), where it can get a bit of exposure to the real world. If nothing comes up it would then be included in an eventual 1.3 release to CRAN. @smbache do you have thoughts on this addition?

If we move forwards, I'd also like to see that the test package works with the unit tests, and to put in one or two tests for this use case.

torfason commented 2 years ago

Closing to trigger new check.

torfason commented 2 years ago

Close and reopen to trigger new check

torfason commented 2 years ago

I checked this out locally, all tests pass and look pretty good overall.

The new test package install causes a bunch of extra text in the test log (see output at bottom), making it harder to examine it (see at bottom of this comment)
Also, it leaves the installed package as an untracked file – should the package file be added to .gitignore – and could it perhaps be installed in a different folder from the test files themselves? (the file is .../import/tests/test_import/packageToTest_0.1.0.tar.gz)

I've rebased it onto the tip of torfason/import, to include fixes to the GitHub check actions, and the check now passes there, see branch https://github.com/torfason/import/tree/prx/awong234-getNamespaceExports

Thoughts?

✓ | F W S  OK | Context
✓ |         1 | Setting up custom test harness                                                  
✓ |         1 | test_basetemplate.R [0.7s]                                                      
⠏ |         0 | test_from.R                                                                     
* installing *source* package ‘packageToTest’ ...
** using staged installation
** R
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (packageToTest)
✓ |         1 | test_from.R [2.6s]                                                              
✓ |         1 | test_into_and_here.R [1.0s]                                                     
✓ |         1 | test_module_directories.R [1.0s]                                                
✓ |         1 | test_module_recursive.R [1.1s]                                                  
⠏ |         0 | List any skipped tests

torfason commented 2 years ago

On closer inspection:

There is already clutter in the test_import dir (modules and such). One extra file is no problem.
Extra text was just a matter of specifying quiet=TRUE in install.packages(). I just did that minor change myself.
Instead of ignoring the .tgz file, I tried adding to repo and only building if missing. Seems to work on all platforms, should incrementally speed up tests, and means that clone->build->check does not result in uncommitted local changes in git.

Current state is in: https://github.com/torfason/import/tree/prx/awong234-getNamespaceExports

Great fix!

torfason commented 2 years ago

Fix has been applied to dev branch (https://github.com/torfason/import/tree/dev)

torfason commented 2 years ago

These changes should now all be in the dev branch rticulate/import@dev (was manually merged), so I'm closing this PR.

Thanks so much for the contribution! (and holler if anything seems missing)

rticulate / import

Update `from.R` to use `getNamespaceExports` at the appropriate time #55

Issue

Expected behavior

Actual behavior

Error Details

Solution

Reprexes

Success reprex