mdlincoln / clipr

R functions for reading and writing from the system clipboard
http://matthewlincoln.net/clipr/
151 stars 24 forks source link

Response to CRAN #30

Closed jennybc closed 6 years ago

jennybc commented 6 years ago

cc @hrbrmstr @MilesMcBain

Bob, Miles, and I all got this email from Prof Brian Ripley recently:

CRAN packages leaving processes running which is disallowed by the CRAN policy.

Packages

datapasta reprex splashr

leave instances of xsel running on Fedora Linux and xclip (x2) on Solaris. And that means that make-based package checking thinks the check has not finished until the rogue processes are killed manually, so is a considerable nuisance (but because 3 packages are involved, it has taken quite a while to isolate the cause, which is invoking clipr::write_clip (which clipr itself does not currently do).

vegalite contains similar code but does not exercise it.

Beyond this the CRAN team have discussed package access to the user's clipboard and feel that it falls in to the same category as access to the file system, that is should only be done in interactive sessions with the user's permission (especially but not only write access).

Please correct ASAP and before Jan 24 to safely retain the package on CRAN.

I'm not working on this yet but we should coordinate our response. It is possible we might need/want something in clipr itself re: checking for interactive session and not being in tests. Regardless, it probably makes sense to discuss here.

mdlincoln commented 6 years ago

Thanks @jennybc. I'm unsure whether this means they want those three packages to NOT invoke clipr::write_clip, or whether they want changes in clipr itself to prevent operation in a non-interactive session?

We could add an argument to read/write clip with allow_non_interactive = FALSE that defaults to preventing use, but can be overridden. I hesitate to hardcode logic into clipr that completely prevents it from being used in non-interactive sessions - this seems like a decision that the user should get to make.

May also warrant editing the "Developing with clipr" section of the README file.

hrbrmstr commented 6 years ago

I’m going to try to debug this weekend (I think I can come close to replicating CRAN’s test env in Docker).

But, (IMO) we shld all avoid testing clipboard-y things on CRAN.

On Dec 29, 2017, at 2:38 PM, Matthew Lincoln notifications@github.com wrote:

Thanks @jennybc. I'm unsure whether this means they want those three packages to NOT invoke clipr::write_clip, or whether they want changes in clipr itself to prevent operation in a non-interactive session?

We could add an argument to read/write clip with allow_non_interactive = FALSE that defaults to preventing use, but can be overridden. I hesitate to hardcode logic into clipr that completely prevents it from being used in non-interactive sessions - this seems like a decision that the user should get to make.

May also warrant editing the "Developing with clipr" section of the README file.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jennybc commented 6 years ago

(IMO) we shld all avoid testing clipboard-y things on CRAN

Agreed. I believe that was my intent but I haven't checked yet to see exactly how I am violating this.

jennybc commented 6 years ago

I'm unsure whether this means they want those three packages to NOT invoke clipr::write_clip, or whether they want changes in clipr itself to prevent operation in a non-interactive session?

I think if they wanted a change in clipr itself, you @mdlincoln would have gotten an email. I think it's the other packages that need to respond. But it's possible that there's some addition here that would help us all. In any case, whatever we do, it should be recorded as a best practice for using clipr in a CRAN package.

MilesMcBain commented 6 years ago

@mdlincoln I like your idea. It's simple and most of all kind to the CRAN maintainers. So it would error if used in a non-interactive session without the opt-in?

For datapasta, I think I will rejig my test skips such that all the tests with clipboard operations are skipped when in non-interactive mode. I'm wondering if I should also leave skip_on_CRAN in place as a safety. Thoughts anyone?

MilesMcBain commented 6 years ago

Hey friends, I am wondering if this CRAN policy change will necessitate additional changes to those we have discussed. See: https://github.com/eddelbuettel/crp/commit/0c28586924a50c1ee545607192a4a10b3e165ebf

The policy seems to be saying writing to the clipboard is out of bounds with the qualification:

Limited exceptions may be allowed in interactive sessions if the package obtains confirmation from the user.

Edit: rereading Prof Brian Ripley's email in the context of this update, the last para takes on a new flavour. Now I wonder if they want us to request any access to the clipboard from the user?

jennybc commented 6 years ago

I will consult with some colleagues and be back in touch.

hadley commented 6 years ago

If the user is an interactive session, I think it will be ok to copy to the clipboard if it is clear that that is the primary purpose of the function. For example, copying to the clipboard is the primary means by which reprex::reprex() communicates back to the user, so no addition permission is required.

It's a bit trickier for non-intearctive sessions - I'd say that you should not copy to the clipboard by default. Perhaps the easiest way is to add a copy_to_clipboard = interactive() function. If you don't think that is sufficient, I'd suggest adding clipr::copying_ok() or similar which also consulted a global option (and perhaps was automatically disabled on CRAN).

mdlincoln commented 6 years ago

I'm sorry to say other priorities meant I had to leave this fallow for some time. And reading over https://github.com/tidyverse/reprex/issues/171 I guess I'm still a bit confused what CRAN's current thinking is on this subject?

I'd like to push a small clipr update to address #31. Do we feel that coding in something explicitly to clipr is warranted, or would advice in the README about skipping tests on cran be sufficient?

mdlincoln commented 6 years ago

Added advice to README to not use in non-interactive session.