Skip to content
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

Response to CRAN #30

Closed
jennybc opened this issue Dec 29, 2017 · 10 comments
Closed

Response to CRAN #30

jennybc opened this issue Dec 29, 2017 · 10 comments
Assignees

Comments

@jennybc
Copy link
Contributor

jennybc commented Dec 29, 2017

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
Copy link
Owner

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
Copy link

hrbrmstr commented Dec 30, 2017 via email

@jennybc
Copy link
Contributor Author

jennybc commented Dec 30, 2017

(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
Copy link
Contributor Author

jennybc commented Dec 30, 2017

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
Copy link
Contributor

@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
Copy link
Contributor

MilesMcBain commented Jan 15, 2018

Hey friends, I am wondering if this CRAN policy change will necessitate additional changes to those we have discussed. See: eddelbuettel/crp@0c28586

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
Copy link
Contributor Author

jennybc commented Jan 15, 2018

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

@hadley
Copy link

hadley commented Jan 15, 2018

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
Copy link
Owner

I'm sorry to say other priorities meant I had to leave this fallow for some time. And reading over tidyverse/reprex#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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants