ropensci / rdataretriever

R interface to the Data Retriever
https://docs.ropensci.org/rdataretriever
Other
44 stars 21 forks source link

Retriever installers need to setup paths for CLI #5

Closed ethanwhite closed 10 years ago

ethanwhite commented 10 years ago

At the moment the retriever's installers for Windows and (I'm fairly sure) Mac don't automatically add the retriever to the path (source installs handle this properly). For this package to work seamlessly this needs to be addressed. This issue is just a flag to remind us that https://github.com/weecology/retriever/issues/174 needs to be handled before we finish this.

dmcglinn commented 10 years ago

Good call on this issue. For now I suppose we should add instructions to the README.md file for how to mannualy do this.

dmcglinn commented 10 years ago

On windows (possibly on Mac as well) I found a bit of a work around. It's not ideal because the path must be set each time R is run but it is a quick and dirty solution for now.

## first check that retriever is not on the current PATH
grepl('EcoDataRetriever', Sys.getenv('PATH'))
#[1] FALSE
## now add the default windows path of the retriever to the existing path
newpath = paste(Sys.getenv('PATH'), 'C:\\Program Files\\EcoDataRetriever', sep=';')
Sys.setenv('PATH' = newpath)
## now attempt a call to retriever using system()
system('retriever -h')
dmcglinn commented 10 years ago

@sckott if we were to try to set the path as I have described above for Windows users when the package loads is your approach of with the rgbif package of using a startup.r file a reasonable way to go? Alternatively this could be added into a zzz.r file I suppose. Thanks!

sckott commented 10 years ago

Hmm, @dmcglinn I've not done this kind of thing before. I want to make sure we're not doing it wrong.

@gavinsimpson Thoughts on this? That is, setting path to retriever on OSX so that it is detected in RStudio?

gavinsimpson commented 10 years ago

@sckott I'd be wary of fiddling with the path in anything that will go to CRAN; you should be able to expect that the user has installed the specified SystemRequirements needed to allow the packager to run.

@dmcglinn There are two things that happen when packages are "loaded" into R, they get loaded (well the NAMESPACE does) and they can also be attached. In normal use, require("foo") or library("foo") both load and attach the stated package, foo. However, if a package, Alice, imports functions from package Bob, then when Alice is needed Alice gets loaded and attached but Bob only gets loaded. Hence depending on what you want to achieve you will need to add a hook using on.load() or on.attach(). I suspect you'd want on.load() because you want to have retriever work whether it was loaded or attached.

Just be wary of fiddling with the path when loading packages. If this is long term, and you intend to send to CRAN at some point, check their policy and drop them an email to enquire about acceptability of path-fiddling. If this is short-term (and it sounds like it is), I wouldn't bother adding to the package. Just include the work around instructions in README until the installers sort the path bits out.

sckott commented 10 years ago

Okay, thanks @gavinsimpson ! Yeah, @cboettig mentioned adding info to the SystemRequirements line #11

ethanwhite commented 10 years ago

I've done some looking around and believe I know how to have the retriever add itself to the path on Windows when installed via the installer. Barring unanticipated difficulties this can go into the next release.

I don't know how to do this for Mac yet, and it seems that in OS X even when it is built from source, and is therefore in the path, it still doesn't work properly in RStudio (#10). Any Mac packaging gurus on the rOpenSci team?

dmcglinn commented 10 years ago

We have added text to README.md that describe to the user how to add the path to the retriever for windows we still need to add Mac instructions. Are their any Mac folks that can help us with this?

sckott commented 10 years ago

@dmcglinn I'll try to add some to the README.md along lines of the windows instructions

dmcglinn commented 10 years ago

Thanks @sckott!

sckott commented 10 years ago

I just tested this out on my machine, reinstalling the app from source, (fixing some python path, my bad), then installing the R wrapper. I actually didn't need to add any paths within R. It just worked after loading the package.

Maybe @emhart or @karthik can see if they have the same experience

karthik commented 10 years ago

Any Mac packaging gurus on the rOpenSci team?

Unfortunately not me. But I will test it out shortly and let you know if I have any problems.

dmcglinn commented 10 years ago

I just got a bug report from a Mac user that installed the Ecological data retriever: https://github.com/weecology/retriever/releases/download/v1.6.0/retriever-app.zip (i.e., they did not build from source) and it appears that the Data Retriever was not added to the path because the following error was returned sh: retriever: command not found while trying to use the R package.

sckott commented 10 years ago

@dmcglinn Did the user verify that retriever works on the CLI?

dmcglinn commented 10 years ago

Good question. Now the user has verified that the 'retriever' does NOT work from the CLI.

Dan On May 9, 2014 3:01 PM, "Scott Chamberlain" notifications@github.com wrote:

@dmcglinn https://github.com/dmcglinn Did the user verify that retrieverworks on the CLI?

— Reply to this email directly or view it on GitHubhttps://github.com/ropensci/ecoretriever/issues/5#issuecomment-42713386 .

sckott commented 10 years ago

Hmm, perhaps a Python problem for that user?

ethanwhite commented 10 years ago

Sorry I've been slow in responding. Data Carpentry + Moore round two are keeping me busy. I think this is all the expected behavior at this point. Source installs will add themselves to the path, but the installers do not (hence this issue). The Retriever likely works fine but just can't be found since it's not in the path. On May 9, 2014 7:26 PM, "Scott Chamberlain" notifications@github.com wrote:

Hmm, perhaps a Python problem for that user?

— Reply to this email directly or view it on GitHubhttps://github.com/ropensci/ecoretriever/issues/5#issuecomment-42723722 .

ethanwhite commented 10 years ago

This is now solved for the Windows installer in https://github.com/weecology/retriever/pull/189. Now we either need a good solution for Mac or (more likely since I've so far failed to find anything promising) document this prominently for Mac users.

ethanwhite commented 10 years ago

I have added detailed instructions for OS X users to the Retriever's download page on how to add the Retriever to the path: http://ecodataretriever.org/download.html It turns out this has to be done in a very specific way to avoid potential issues with RStudio mentioned in #10 and I have documented these in the form of a one-liner that can simply be pasted into a terminal.

If some Mac folks have a few minutes to test that these instructions work on clean systems (i.e., if you've already installed the Retriever from source run sudo pip uninstall retriever once or twice until it reports that it is not installed) by running system('retriever ls') from inside R and RStudio that would be great.

Now we are just a new Retriever release (for the Windows installer update) away from solving this at which point I think we're getting close to a first release!

sckott commented 10 years ago

@ethanwhite You're missing a single quote at the end I think. Should be

sudo -s 'echo "/Applications/retriever.app/Contents/MacOS" > /etc/paths.d/retrieverapp'

Doesn't seem to work for me. getting on retriever ls

Traceback (most recent call last):
  File "/usr/local/bin/retriever", line 5, in <module>
    from pkg_resources import load_entry_point
  File "/usr/local/lib/python2.7/site-packages/pkg_resources.py", line 2749, in <module>
    working_set = WorkingSet._build_master()
  File "/usr/local/lib/python2.7/site-packages/pkg_resources.py", line 444, in _build_master
    ws.require(__requires__)
  File "/usr/local/lib/python2.7/site-packages/pkg_resources.py", line 725, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/usr/local/lib/python2.7/site-packages/pkg_resources.py", line 628, in resolve
    raise DistributionNotFound(req)
pkg_resources.DistributionNotFound: retriever==1.6
ethanwhite commented 10 years ago

Fixed the missing quote. Thanks.

The error you're getting is from the CLI or RStudio? Which version of OS X?

sckott commented 10 years ago

sorry,

ethanwhite commented 10 years ago

Looks like it's still trying to access your old source installation. Can you check which retriever and if it is /usr/local/bin/retriever do an rm /usr/local/bin/retriever and try again?

ethanwhite commented 10 years ago

Hey @sckott - we're getting ready to roll out a new Retriever release and to actually release the R package as well. Can you take a look at your previous issue and see if my last comment fixes it? Thanks!

sckott commented 10 years ago

@ethanwhite yep, will do in a bit here, in the middle of a few things...

sckott commented 10 years ago

@ethanwhite I'm getting errors on installation step. When I try:

devtools::install_github("ropensci/ecoretriever")

On installation the pkg is trying to run the .onLoad function I imagine, which is throwing a popup alert box. See image.

I did the path thing on the retriever download page first, which worked fine. I'm probably missing something obvious

dmcglinn commented 10 years ago

It seems like this error was generated by the retriever not being on your path because the point of failure in the installation is when the package is being initialized for the first time. During package initialization the command retriever update is issued to update all the data download scripts and I think is causing your error.

sckott commented 10 years ago

Hmm, I guess the setting the path part didn't work then....

I ran

sudo -s 'echo "/Applications/retriever.app/Contents/MacOS" > /etc/paths.d/retrieverapp'

and this shows

~$ which retriever
/Applications/retriever.app/Contents/MacOS/retriever

Is that right?

ethanwhite commented 10 years ago

That all looks correct Scott. What happens if you try to run the retriever from the command line? Just try: retriever --help And retriever update

On Friday, September 5, 2014, Scott Chamberlain notifications@github.com wrote:

Hmm, I guess the setting the path part didn't work then....

I ran

sudo -s 'echo "/Applications/retriever.app/Contents/MacOS" > /etc/paths.d/retrieverapp'

and this shows

~$ which retriever /Applications/retriever.app/Contents/MacOS/retriever

Is that right?

— Reply to this email directly or view it on GitHub https://github.com/ropensci/ecoretriever/issues/5#issuecomment-54690811.

sckott commented 10 years ago

Weird, nope, that doesn't work. I get the same alert error popup as before https://github.com/ropensci/ecoretriever/issues/5#issuecomment-54682844

I'll try again in the morning

ethanwhite commented 10 years ago

OK, what does ls -l /Applications/retriever.app/Contents/MacOS/ give you?

sckott commented 10 years ago
total 208
-rwxr-xr-x@ 1 sacmac  staff  24960 Feb  6  2014 python
-rwxr-xr-x@ 1 sacmac  staff  74832 Feb  6  2014 retriever
ethanwhite commented 10 years ago

I don't see anything here to indicate a path problem. @dmcglinn what was it that suggested a path issue to you?

dmcglinn commented 10 years ago

@ethanwhite I just assumed it was a path problem because it appeared that retriever was not responding to the system() call within R. My understanding now based on the fact that @ethanwhite thinks @sckott path looks good that the retriever just didn't install properly. Does the GUI work @sckott or is that broken too?

ethanwhite commented 10 years ago

OK, so, there isn't a path issue here. That appears to all be working fine. The GUI won't work if the CLI doesn't.

@sckott can you run /Applications/retriever.app/Contents/MacOS/retriever update and post the resulting error (there should be some output to the terminal) over in the Retriever repo. Thanks.

ethanwhite commented 10 years ago

This issue has gotten pretty far off track and now mixes multiple topics. Since the core issue has been addressed I'm closing this. Let's pick up @sckott's issues over in the core repository.

sckott commented 10 years ago

@ethanwhite where exactly?

ethanwhite commented 10 years ago

Just as a new issue please. This appears to be a problem with the core retriever installation.

sckott commented 10 years ago

okay, see weecology/retriever#199