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

problem setting globals in .onLoad() #3

Closed
jcdny opened this Issue Jan 24, 2011 · 11 comments

Comments

Projects
None yet
3 participants
@jcdny

jcdny commented Jan 24, 2011

I have a package which sets some globals in the .onLoad function but when I try to load the package with devtools I get an error trying to reference the global variables within the .onLoad function itself.

simple example:

.onLoad <- function(lib, pkg, ...) {
  XX <<- "key"
  YY <<- paste(XX, "value",sep="=")
}

Produces the following error in R

$ R
> l(gmutil)
Error in paste(XX, "value", sep = "=") : object 'XX' not found

if I comment out the second line the code loads fine and XX is set in the global environment.

The package itself passes R CMD check with no errors an loads fine when installed...

@hadley

This comment has been minimized.

Member

hadley commented Jan 25, 2011

The problem is that I have no control over <<- - it's always going to work in the global environment, instead of the environment in which devtools is loading all the other code. Could you provide a bit more info about what you're doing with the globals? There might be a better way to set them up.

@jcdny

This comment has been minimized.

jcdny commented Jan 25, 2011

The code keeps a list of database environment variables and a key for
which to DB use. It's in the global environment since the package is
also used in some legacy code which sets these explicitly prior to the
package being loaded.

Here is the actual .onLoad function....

.onLoad <- function(lib, pkg, ...) {
  ## CONNECTION MANAGEMENT
  if (!exists(".DB.USE",globalenv()))
    .DB.USE <<- Sys.getenv("DB_USE", unset="PROD")

  if (!exists(".DB.ENV",globalenv())) {
    .DB.ENV <<- list()
    .DB.ENV[[.DB.USE]] <<- db.env.get(.DB.USE)
  }
}

I changed it to use the package namespace (following the example in
lubridate):

.DB.ENV <- NULL
.DB.USE <- NULL

.onLoad <- function(lib, pkg, ...) {
  ## CONNECTION MANAGEMENT
  if (is.null(.DB.USE))
    .DB.USE <<- Sys.getenv("DB_USE",unset="PGPROD")

  if (is.null(.DB.ENV)) {
    .DB.ENV <<- list()
    .DB.ENV[[.DB.USE]] <<- db.env.get(.DB.USE)
  }
}

Which is cleaner, works, and is no doubt the right answer but that
means I needed to explicitly unlock the bindings for .DB.USE and
.DB.ENV, and the existing legacy code would need to be modified to
work properly (overdue cleanup so not terrible I guess).

It just surprised me that a package that worked fine via library would not load.
Looking at devtools code I am guessing the issue is that when you create
the environment you don't assign a parent frame and then do the attach
before sourcing the code rather than sourcing then attaching.

@jcdny

This comment has been minimized.

jcdny commented Jan 25, 2011

I accidentally closed this. I made the change to create the environment, source, then attach and then the sample code works. Take a look at jcdny/devtools@025c650 for what I am talking about.

@hadley

This comment has been minimized.

Member

hadley commented Jan 26, 2011

You've made two changes in that code - one to source then attach, and the other to use global environment as the parent. Would you mind testing to figure out which is the important change?

@jcdny

This comment has been minimized.

jcdny commented Jan 26, 2011

You need to do both things. You need to provide the enclosing environment when doing the source and when you attach it actually makes a copy of the environment and I think discards the enclosing environment.

I've messed around with it a bit and nothing seems to break with that change though I admit it was all a bit cargo culted and it's not clear if the env should be globalenv() or
something else. Someone with much more R internals experience would have to answer that one.

@jcdny

This comment has been minimized.

jcdny commented Jan 26, 2011

Here is a simple example of what I am talking about

In /tmp/x.R:
ZZ <<- 1
cat("found ZZ in",find("ZZ"),"\n")
cat("ZZ=",ZZ,"\n")

then run:

e1 <- new.env(parent = emptyenv())
attach(e1, name="E1")
e2 <- as.environment("E1")
e3 <- new.env(parent = globalenv())
attach(e3, name="E3")
e4 <- as.environment("E3")


for (e in c("e1","e2","e3","e4")) {
  cat("\n",e,"\n")
  print(get(e))
  try(sys.source("/tmp/x.R", env=get(e)))
  try(rm("ZZ", envir=globalenv()))
}

Which for me produced:
e1
<environment: 0x2873178>
Error in eval(expr, envir, enclos) : could not find function "<<-"

 e2 
<environment: 0x28728a0>
attr(,"name")
[1] "E1"
found ZZ in .GlobalEnv 
Error in cat("ZZ=", ZZ, "\n") : object 'ZZ' not found
In addition: Warning message:
In rm("ZZ", envir = globalenv()) : object 'ZZ' not found

 e3 
<environment: 0x1fc4160>
found ZZ in .GlobalEnv 
ZZ= 1 

 e4 
<environment: 0x1fc38f8>
attr(,"name")
[1] "E3"
found ZZ in .GlobalEnv 
Error in cat("ZZ=", ZZ, "\n") : object 'ZZ' not found
@hadley

This comment has been minimized.

Member

hadley commented Jan 29, 2011

Thanks Jeff - I'll incorporate that change when I get a chance

@hadley

This comment has been minimized.

Member

hadley commented Jan 29, 2011

Hmmmm, this change breaks with S4 because of the way method tables work - the environment needs to be attached before the class definition functions are run,

@jcdny

This comment has been minimized.

jcdny commented Jan 29, 2011

Ah, I tested with a number of packages but nothing with S4 methods. That's unfortunate.

I will think about it more and if I get any bright ideas I will let you know. I suspect to support namespaces you will need to load packages more like the existing code does and that might be the only way to get both of these.

@hadley

This comment has been minimized.

Member

hadley commented Feb 4, 2011

Yes, it's hard to replicate regular package loading exactly because so many of the key functions are internal, and nothing is really documented. One day I'll be able to do better!

@wch

This comment has been minimized.

Member

wch commented Aug 10, 2012

Fixed in #126.

@wch wch closed this Aug 10, 2012

dy-kim added a commit to dy-kim/devtools that referenced this issue Mar 31, 2017

dy-kim added a commit to dy-kim/devtools that referenced this issue Jun 28, 2017

@lock lock bot locked and limited conversation to collaborators Sep 19, 2018

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