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

CRAN having problems on Fedora #38

Closed
mdlincoln opened this Issue Jan 8, 2019 · 25 comments

Comments

Projects
None yet
6 participants
@mdlincoln
Copy link
Owner

mdlincoln commented Jan 8, 2019

BDR email below (FYI @jennybc @MilesMcBain I'll be looking into it this week)

The CRAN policy says

- Packages should not write in the user’s home filespace (including clipboards), nor anywhere else on the file system apart from the R session’s temporary directory (or during installation in the location pointed to by TMPDIR: and such usage should be cleaned up). Installing into the system’s R installation (e.g., scripts to its bin directory) is not allowed.

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

clipr contains a function write_clip() which does not comply. It is
exercised in its tests and despite the claim on its help page, on Fedora
leaves an xsel process running which blocks the check run from
completing. As a result we have to use --no-tests, and every time some
other package uses write_clip it too blocks the check run.

To comply with the policy, please ensure that write_clip() obtains the
user's permission and never runs in a non-interactive session. And
modify your help and tests.

Packages which violate the CRAN policy are normally removed immediately.
As several packages depend on clipr we will give you until Jan 22 to
correct this.

--
Brian D. Ripley, ripley@stats.ox.ac.uk
Emeritus Professor of Applied Statistics, University of Oxford

@mdlincoln mdlincoln changed the title CRAN havign problems on Fedora CRAN having problems on Fedora Jan 8, 2019

@mdlincoln mdlincoln self-assigned this Jan 8, 2019

@mdlincoln mdlincoln added the bug label Jan 8, 2019

@jennybc

This comment has been minimized.

Copy link
Contributor

jennybc commented Jan 8, 2019

I think the main thing is to make sure that no code that exercises the clipboard ever runs on CRAN. At all. Ever. And that it would be impossible for a user to NOT understand that clipr may access their clipboard, i.e. clipr usage is effectively consent.

Given the name, description, etc. of clipr, I think it will likely continue to be tolerated as a "limited exception", if you can ensure that it does not cause grief for CRAN by leaving, e.g., an xsel process behind.

Have you identified what is tickling xsel in the first place during check?

@MilesMcBain

This comment has been minimized.

Copy link
Contributor

MilesMcBain commented Jan 9, 2019

Hard not to be triggered by this. To give you until Jan 22 is quite cruel. Kind of assumes you would be privileged enough to be able to spend work hours on it. Boo.

@MilesMcBain

This comment has been minimized.

Copy link
Contributor

MilesMcBain commented Jan 9, 2019

So I took a look at the xsel stuff again since I was the last one to touch it.

This bit of the xsel man page is relevant:

 There  is no X selection buffer. The selection mechanism in X11 is an interclient com‐
       munication mediated by the X server each time any program wishes to know the selection
       contents,  eg. to perform a middle mouse button paste. In order to implement modifica‐
       tion of the selection(s) (in input, keep and exchange  modes)  this  program  detaches
       from  the terminal, spawning a child process to supply the new selection(s) on demand.
       This child exits immediately when any other program takes over the  selection(s),  eg.
       when the user next selects some text in a terminal window or by running xsel -c.

So that child process is the one sticking around. I can reproduce it on my Ubuntu machine. The thing is under normal usage the xsel child gets nuked the next time the clipboard is used by another application. I've confirmed this is the case. So I don't know why it's only Fedora, but you'll get a lingering process anytime the last thing you do with the clipboard is write to it with xsel.

Now there is a mode, --nodetach, that stops the child process being created but this locked up my RStudio R session, so I don't think that's an option.

Nor does the mentioned xsel -c work as cleanup for us because we must use with -b and that kills that whole clipboard selection, meaning it cant be pasted anywhere.

So I don't think there's much else you can do other than skip_on_cran() the write tests, I already went that way with datapasta. That wont resolve the issue of other packages calling write_clip() in a test though. To stop that you might want to consider refusing to write if interactive() is FALSE, that makes your tests useless though which is a bit shitty.

@MilesMcBain

This comment has been minimized.

Copy link
Contributor

MilesMcBain commented Jan 9, 2019

Could write_clip() detect it is on CRAN?

@mdlincoln

This comment has been minimized.

Copy link
Owner Author

mdlincoln commented Jan 10, 2019

Thanks both so much for your thoughts. And yes, this explication is really useful @MilesMcBain (also re #36).

It seems to me that they are adamant that write_clip() not run in a non-interactive session - which I certainly understand, save that it makes testing quite ridiculous. If I set write_clip to error if run when !interactive(), I may introduce an argument noninteractive=FALSE to write_clip that would allow us to explicitly call the function during tests when not on CRAN, we could at least still benefit from Travis/Appveyor.

@jennybc

This comment has been minimized.

Copy link
Contributor

jennybc commented Jan 10, 2019

https://github.com/tidyverse/reprex/blob/03eac22fd3bec4de55ff7f01bbaf6dc243cc0cfb/tests/testthat/helper.R#L5-L9

https://github.com/tidyverse/reprex/blob/4705ca9dc666f1a4d42e5dae870e4842d73825f3/R/utils-interactivity.R#L1-L7

I have effectively made sure that I don't touch the clipboard on CRAN. God, at least I hope I have. Maybe some of the above could be brought into clipr itself.

I think it would be a real sacrifice of quality for everyone if it became structurally impossible to use clipr non-interactively. I like being able to test my clipboard functionality fully, locally and on travis and appveyor. I just accept we can't do that on CRAN.

@mdlincoln

This comment has been minimized.

Copy link
Owner Author

mdlincoln commented Jan 10, 2019

@jennybc yeah, I definitely see where you are coming from. Would it be more user-friendly to have allow_non_interactive to check for an environment variable (like your CLIPBOARD_AVAILABLE for example), so that package builders wouldn't have to dig all the way in to change function arguments, but instead just set that envvar when they want to run their tests off CRAN?

@jennybc

This comment has been minimized.

Copy link
Contributor

jennybc commented Jan 10, 2019

Yeah I definitely think this should be controlled by an option or env var, not an argument. This reduces the clutter and also makes the "CRAN kill switch" easier to identify and flip.

@mdlincoln

This comment has been minimized.

Copy link
Owner Author

mdlincoln commented Jan 10, 2019

@jennybc do you have a strong opinion about R option vs. envvar? To be honest, I've never quite understood best practices for when you would choose one over the other...

@jennybc

This comment has been minimized.

Copy link
Contributor

jennybc commented Jan 10, 2019

I'm going to invite others to chime in. Please hold.

@jimhester

This comment has been minimized.

Copy link

jimhester commented Jan 10, 2019

I think something like enabled = Sys.getenv("CLIPR_ENABLE", interactive()) would be ideal.

It should preserve the interactive case as before, but be disabled during R CMD check on CRANs machines, and if authors wanted to enable clipr on a CI system they can set the envvar.

@gaborcsardi

This comment has been minimized.

Copy link

gaborcsardi commented Jan 10, 2019

Why not just clean up the external processes? processx/ps can do that.

@kevinushey

This comment has been minimized.

Copy link

kevinushey commented Jan 10, 2019

What if another application is attempting to use the X11 clipboard through xsel / xclip? It seems like that would cause issues there.

Since xsel basically tries to run as a daemon I think the best fix is just to make sure it never gets run on CRAN altogether rather than try to manage its lifetime. (My understanding is that attempting to 'copy to the clipboard' with X11 really means 'create a process that can serve clipboard requests as needed to all active applications', and so you normally don't want to clean up the underlying process.)

@gaborcsardi

This comment has been minimized.

Copy link

gaborcsardi commented Jan 10, 2019

@kevinushey The xsel process that we started can only hold the selection we send to it. E.g. if you just select sg. in a terminal, our xsel process quits automatically. There is no harm cleaning up our xsel process.

AFAICT xclip works the same way.

@kevinushey

This comment has been minimized.

Copy link

kevinushey commented Jan 10, 2019

I think this would be surprising though, e.g.

  1. Launch R,
  2. Run clipr::write_clipboard("hello"),
  3. Close R,
  4. Attempt to paste the clipboard to some other program.

In (4), if the xsel / xclip process was closed along with R in (3), you'll no longer be able to paste the original clipboard text, which IMHO doesn't feel right.

@mdlincoln

This comment has been minimized.

Copy link
Owner Author

mdlincoln commented Jan 10, 2019

In (4), if the xsel / xclip process was closed along with R in (3), you'll no longer be able to paste the original clipboard text, which IMHO doesn't feel right.

this has been my concern about trying to solve this via process management.

@gaborcsardi

This comment has been minimized.

Copy link

gaborcsardi commented Jan 10, 2019

I think this would be surprising though, e.g.

I meant cleaning up at the end of the tests. You probably don't want to keep the selection that you copied in the tests?

@kevinushey

This comment has been minimized.

Copy link

kevinushey commented Jan 10, 2019

Ahhh, okay. Sorry for being dense; that seems reasonable (although still 'scary' relative to just never running clipboard tests on CRAN)

@jennybc

This comment has been minimized.

Copy link
Contributor

jennybc commented Jan 10, 2019

I strongly believe there should never be an attempt to read or write the clipboard on CRAN.

@mdlincoln

This comment has been minimized.

Copy link
Owner Author

mdlincoln commented Jan 10, 2019

Alright, I think the importance of not having CRAN jeopardize this package again overrides the annoyance of not being able to test on CRAN's infrastructure. I worry that any process-fudging solution will be quite fragile - even in this case, it's quite odd that it's only one linux build that is doing this, why aren't the others? And I don't have the time, or, frankly, the knowledge base to spend debugging xsel's behavior solely to comply with CRAN directive.

For that reason, I'm going to go with completely avoiding tests on CRAN by only allowing write_clip() in non-interactive settings when the env var ALLOW_CLIPBOARD is set to TRUE.

@jennybc

This comment has been minimized.

Copy link
Contributor

jennybc commented Jan 10, 2019

it's quite odd that it's only one linux build that is doing this, why aren't the others?

As @jimhester pointed out elsewhere, it also depends on the flags CRAN is using, e.g. whether --no-tests is set or not. That could vary by platform and over time.

the env var ALLOW_CLIPBOARD

Just on principle, I think the env var's name should include "clipr". Maybe CLIPR_CLIPBOARD_AVAILABLE or some such.

@gaborcsardi

This comment has been minimized.

Copy link

gaborcsardi commented Jan 10, 2019

For that reason, I'm going to go with completely avoiding tests on CRAN by only allowing write_clip() in non-interactive settings when the env var ALLOW_CLIPBOARD is set to TRUE.

This does break the CRAN checks for existing packages that use clipr in their test cases or examples, though, right?

@MilesMcBain

This comment has been minimized.

Copy link
Contributor

MilesMcBain commented Jan 10, 2019

@gaborcsardi They should all have cleaned those up after Episode 1 of Fun with CRAN and Clipboards almost exactly a year ago. A lingering xsel processes on Fedora and xclip process on Solaris were cited back then in BDR's original letter of indictment.

I think the proposed change makes sense.

@mdlincoln

This comment has been minimized.

Copy link
Owner Author

mdlincoln commented Jan 10, 2019

I just ran revdep_check() after trying out adding an error on !interactive() and got no errors from anyone so it would seem they either weren't testing code that invoked write_clip or, like reprex, datapasta, readr, and others, made sure to check for clipboard availability as instructed.

@mdlincoln

This comment has been minimized.

Copy link
Owner Author

mdlincoln commented Jan 11, 2019

clipr 0.5.0 is on its way to CRAN - hopefully these modifications pass muster with them. Thanks all for your very helpful input.

@mdlincoln mdlincoln closed this Jan 12, 2019

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