zoonproject / zoon

The zoon R package
Other
61 stars 13 forks source link

ZOON archived: Installing packages not allowed #403

Closed AugustT closed 6 years ago

AugustT commented 6 years ago

CRAN have archived zoon, which I believe means it is unavailable to anyone to install.

The issue seems to be that they don’t like the GetPackage function which installs stuff, behind the scenes, without the user knowing. They don’t seem happy about it.

I don’t have time to deal with this at the moment. I'm trying to think what would be the easiest fix. I think we could remove GetPackage from checks (that is what caused CRAN problems), and we could require GetPackage to get user permission before installing by waiting on user input (e.g. 'This requires the package "gam" which is not installed, do you want to install it now? Y/N')

@goldingn @timcdlucas @GregMci

>Dear maintainer,
>
>while checking your package, we observed:
>
>
>
> > GetPackage('gam')
>Loading required package: gam
>Warning in library(package, lib.loc = lib.loc, character.only = TRUE,
>logical.return = TRUE, :
>   there is no package called 'gam'
>Installing package into 'D:/temp/RtmpMr5zWx/RLIBS_2cc0266088f'
>(as 'lib' is unspecified)
>also installing the dependencies 'iterators', 'foreach'
>
>trying URL
>'http://cran.rstudio.com/bin/windows/contrib/3.5/iterators_1.0.9.zip'
>Content type 'application/zip' length 320703 bytes (313 KB)
>==================================================
>downloaded 313 KB
>
>trying URL
>'http://cran.rstudio.com/bin/windows/contrib/3.5/foreach_1.4.4.zip'
>Content type 'application/zip' length 388583 bytes (379 KB)
>==================================================
>downloaded 379 KB
>
>trying URL 'http://cran.rstudio.com/bin/windows/contrib/3.5/gam_1.14-4.zip'
>Content type 'application/zip' length 319276 bytes (311 KB)
>==================================================
>downloaded 311 KB
>
>package 'iterators' successfully unpacked and MD5 sums checked package
>'foreach' successfully unpacked and MD5 sums checked package 'gam'
>successfully unpacked and MD5 sums checked
>
>
>This violates CRAN policies and polluted my check system. hence I had a lot
>work to do to clean up and find whgere some problem came from.
>
>Never install packages without user interaction.
>
>
>Hence we had to archive your package.
timcdlucas commented 6 years ago

Caret do a similar thing but with a yes no question when packages install. So adding this to getPackages is probably easiest. I've never written user interaction code before but I'll see if I can find it it caret.

timcdlucas commented 6 years ago

https://github.com/topepo/caret/blob/1860514ccc145a7d322bdf73a9381893fada292b/pkg/caret/R/modelLookup.R

It's in the function checkInstall here.

Can't fix anything right now in afraid. If noone gets to it first I'll have a go as soon as I can.

goldingn commented 6 years ago

Bugger. I wish they'd give us a grace period to change stuff before archiving!

Yes, that's the best approach I think. I can't implement either as I'm camping, back at work on 15th.

timcdlucas commented 6 years ago

I'll try and fix this now. But I'm assuming only @AugustT can upload to CRAN as he's the maintainer?

timcdlucas commented 6 years ago

Hi all.

Think the core issue is broadly fixed here: https://github.com/timcdlucas/zoon/blob/fix_getPackage/R/GetPackage.R

But I'm having issues with getting CI to work.

Firstly

> expect_true(x <- zoon:::test_module(paste0(basepath,
+                                            "OneHundredBackground.R")))

and other tests don't work because randomPoints isn't expressed as dismo::randomPoints. Which feels like it should have been an issue before. This is one of those things that's harder because I haven't contributed much recently, so I don't know what state the CI was in.

However, directly relevant is

> expect_true(x <- zoon:::test_module(paste0(basepath,
+                                            "CarolinaWrenPO.R")))

This now requires an interactive input to install maxlike which as far as I understand doesn't work in an automated test. So, I'm unsure how to proceed. That said these tests are all skipped on cran.

There's some other failing tests that I'm not sure if fixing the above will fix them as well. Can't find function is.string, etc.

Anyway, just letting you know how I'm getting on.

AugustT commented 6 years ago

Thanks for this @timcdlucas .

The current master branch passes CI (it occasionally fails but this a CI not zoon issue). So unfortunately if you are seeing failures across all 4 builds this will be something due to the changes you have made. Now that we need user input to install packages any test that would previously have done this automatically will now fail. I thought I had tried as much as possible to remove tests that relied on anything but the base packages, so my bad on that one.

I've had a quick look at the error logs and I think if we can clear this up we can get most of the errors out of the way.

timcdlucas commented 6 years ago

OK. I'm under the cosh at work but I'll try to have another go at some point.

I definitely think the CarolinaWren failure was just needing to have interactive input. Not sure about the others.

AugustT commented 6 years ago

I'll try and give this a go too. Currently struggling with Git! I'm going on holidays 10 -> 20th Jan so won't be around then to help, sorry. I think anyone can submit the update to CRAN, just only I will get the email to confirm, if you Whatsapp me on holiday I will login to my email and can approve.

AugustT commented 6 years ago

I'll focus on removing calls to getpackage from tests

timcdlucas commented 6 years ago

Ah ok. Do you wanna private email me your whatsapp or something just in case?

AugustT commented 6 years ago

I'm trying to pull the upstream version but get

fatal: bad object c5cc00033ea... etc

Is anyone else getting the same?

This thread... https://stackoverflow.com/questions/8788975/bitbucket-git-error-did-not-send-all-necessary-objects ... suggests it could be an issue with the repository...

timcdlucas commented 6 years ago
$ git fetch zoon master
From https://github.com/zoonproject/zoon
 * branch            master     -> FETCH_HEAD

works fine for me?

AugustT commented 6 years ago

Can you try

git fetch zoon
timcdlucas commented 6 years ago

Seems ok

$ git fetch zoon
remote: Counting objects: 88, done.
remote: Total 88 (delta 39), reused 39 (delta 39), pack-reused 49
Unpacking objects: 100% (88/88), done.
From https://github.com/zoonproject/zoon
 * [new branch]      gh-pages   -> zoon/gh-pages
 * [new branch]      parallel   -> zoon/parallel
 * [new tag]         0.5        -> 0.5
 * [new tag]         0.6        -> 0.6
timcdlucas commented 6 years ago

Tried fetching into a clean folder?

AugustT commented 6 years ago

That's what I'm doing now. It's still playing up.

AugustT commented 6 years ago

I can't figure this out. Whenever I try to merge to the upstream master I get errors left right and centre. I might just start all over again and delete my fork

AugustT commented 6 years ago

In the absence of access to change the code myself...

Remove references to graf https://github.com/zoonproject/zoon/blob/c5cc00033ea94adc05d42eb243d3b0e10ca68383/tests/testthat/testsummary.zoonWorkflow.R#L67 https://github.com/zoonproject/zoon/blob/c5cc00033ea94adc05d42eb243d3b0e10ca68383/tests/testthat/testsummary.zoonWorkflow.R#L84

And there is the ref to maxlike you identified already.

goldingn commented 6 years ago

Hey, sorry I was off the radar. Back now and taking a look at this.

@AugustT I think your master branch was quite stale. The best fix would be to either: roll back to the common commit with something like git reset --hard 3d3fab4ff53f (ditching all the commits around 8th Jan) and then roll forward with git fetch upstream and git merge upstream/master; or just delete and re-clone the repo like you said.

I'm just making sure I can replicate the failure on r-hub (rhub::check_for_cran()), then I'll pick up @timcdlucas' work and make those changes to the tests. If I can get that working today, I'll try to submit, otherwise I'll post progress here so you can pick up where I left off.

goldingn commented 6 years ago

Looks like Tim was 99% there - the CarolinaWren error was just that the library() call was in the wrong clause of GetPackage(). This branch is passing all tests locally and meets the CRAN policy. If it passes on Travis and r-hub (1, 2, 3), I'll submit to CRAN

goldingn commented 6 years ago

OK, successes across the board, I'll submit now. @AugustT you should get an authorisation email.

timcdlucas commented 6 years ago

Great work. Sorry I didn't find time to do the final bits of this. Be glad to have it back up!

goldingn commented 6 years ago

No problem, thank you for doing the most important bit!

AugustT commented 6 years ago

Hi Both, great work on getting this fixed, sorry for being away and having a total 'mare with git!

Now submitted to CRAN

AugustT commented 6 years ago

Now on CRAN