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

Release ggplot2 3.1.0 #2890

Closed
clauswilke opened this Issue Sep 12, 2018 · 70 comments

Comments

Projects
None yet
8 participants
@clauswilke
Member

clauswilke commented Sep 12, 2018

Issues to resolve before release:

  • #2895 (brushing in shiny doesn't work for coord_trans())

PRs to complete before release:

Prepare for release:

  • devtools::check_win_devel()
  • rhub::check_for_cran()
  • revdepcheck::revdep_check(num_workers = 4)
  • Polish NEWS
  • If new failures, update email.yml then revdepcheck::revdep_email_maintainers()

Perform release:

  • Bump version (in DESCRIPTION and NEWS)
  • devtools::check_win_devel() (again!)
  • devtools::submit_cran()
  • Approve email

Wait for CRAN...

  • pkgdown::build_site()
  • Tag release
  • Merge RC back to master branch
  • Bump dev version
  • Write blog post
  • Tweet
  • Add link to blog post in pkgdown news menu

Template from r-lib/usethis#338

@josesho

This comment was marked as outdated.

josesho commented Sep 24, 2018

Is #2788 going to be part of ggplot 3.0.1? If not, can I request that it be included? (Working on fixing mission-critical function, and this breaks it in a massive way.)

@clauswilke

This comment was marked as outdated.

Member

clauswilke commented Sep 24, 2018

@josesho If your code works with the current github master then it should work with ggplot2 3.0.1. If it doesn't work with the current github master then please file an issue with reprex.

@josesho

This comment was marked as outdated.

josesho commented Sep 25, 2018

Thanks for the heads-up, the current master does work and fixes the issue. Hopefully y'all can ggplot2 3.0.1 out soon! Looking forward 💪🏻

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 25, 2018

@topepo Could you run revdep_check() on the current ggplot2 master branch, please? All the pending patches have been merged and we are ready to proceed.

@hadley Could you run devtools::check_win_devel()? It looks like only the package maintainer can do it. And could you also run rhub::check_for_cran()? I got some strange error messages about a missing sf package and about long file names, and I'm not sure how to interpret them.

@clauswilke

This comment has been minimized.

@batpigandme

This comment was marked as resolved.

Member

batpigandme commented Sep 25, 2018

Per @jennybc's suggestion in the OG thread: r-lib/usethis#338 (comment), I think it does make sense to do the pkgdown build after CRAN approval

@clauswilke

This comment was marked as resolved.

Member

clauswilke commented Sep 25, 2018

@batpigandme Thanks, I've moved it down.

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 25, 2018

@yutannihilation

This comment has been minimized.

Member

yutannihilation commented Sep 25, 2018

It seems dependency installation fails. I see errors around line 509, though it's not clear which package is wrong.

Error in desc[1L, "Type"] : subscript out of bounds
Calls: ... force -> i.p -> .install.winbinary -> unpackPkgZip -> %in%
In addition: Warning messages:
1: In unzip(zipname, exdir = dest) :
write error in extracting from zip file
2: In unzip(zipname, exdir = dest) :
write error in extracting from zip file
Execution halted

This seems an error from here: https://github.com/wch/r-source/blob/521c90a175d67475b9f1b43d7ae68bc48062d8e6/src/library/utils/R/windows/install.packages.R#L58

@topepo

This comment was marked as outdated.

Member

topepo commented Sep 26, 2018

Done. I'll put in a PR in a bit

Broken (15)

package version error warning note
ActisoftR 0.0.1 +1 1 +1
AneuFinder 1.8.0 -1 +1 1 +2 1
BloodCancerMultiOmics2017 1.0.2 -1 +2 1
dartR 1.0.5 1 +1 1
GenomicDataCommons 1.4.3 -1 2 +1 1
ggridges 0.5.0 +1 1 +1 1
GOexpress 1.14.0 1 4 +1
MCbiclust 1.4.0 +1 1 1
mglR 0.1.0 -1 +1
RITAN 1.4.2 -1 1 +1 3
scDD 1.4.0 +1 2 2
seqsetvis 1.0.2 +2 1 +1
shadowtext 0.0.2 +1 1 +1
tricolore 1.2.0 +1 1 2
tsbox 0.0.3 +1 1
@topepo

This comment was marked as outdated.

Member

topepo commented Sep 26, 2018

revdep db here

clauswilke added a commit that referenced this issue Sep 26, 2018

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 26, 2018

15 packages are newly broken in revdeps. That's not too bad. I suspect most of them will be due to the aesthetic name standardization that we introduced. That's what's causing the problems in my ggridges package. I used an aesthetic point_color and it is now internally converted to point_colour and thus some parts of the code can't find it. I'll fix ggridges and see if I can write recommendations for best practice.

@hadley @batpigandme @thomasp85 @karawoo @dpseidel @yutannihilation If you have time to look at any of the other broken packages to see what the issues are that would be great.

@batpigandme

This comment has been minimized.

Member

batpigandme commented Sep 26, 2018

I pulled out the Newly Brokens and popped 'em in a gist here:
https://gist.github.com/batpigandme/b59dca03bfb53b6e9a52332e306ff736

@yutannihilation

This comment has been minimized.

Member

yutannihilation commented Sep 26, 2018

ggmap's installation error seems due to #2741 and this is already fixed in the dev version of ggmap: dkahle/ggmap@86ccd74

Error in get(".all_aesthetics", envir = asNamespace("ggplot2")) : 
  object '.all_aesthetics' not found
@batpigandme

This comment has been minimized.

Member

batpigandme commented Sep 26, 2018

Spreadsheet so we can keep track of what's been done (à la ggplot2 3.0.0 release) https://docs.google.com/spreadsheets/d/1T0t3dsF_6TPqa-dEztPiM51h-F9FhUWzVIsVeb3sl5A/edit#gid=0

@batpigandme

This comment has been minimized.

Member

batpigandme commented Sep 26, 2018

Shoot, if you don't have an rstudio email, shoot me an email from wherever to my rstudio email (or request access from the googlesheet), and I can add editing powers for you

@yutannihilation

This comment has been minimized.

Member

yutannihilation commented Sep 26, 2018

Compared to @batpigandme's gist, @topepo's summary omits some packages. Here's the packages and reasons (seems ignorable except for ggtern):

  • ggmap: installation error due to #2741 (and is already fixed)
  • ggtern: installation error due to #2741 (needs to be informed to the maintener)
  • derfinderPlot: LaTeX errors when creating PDF version.
  • gifski: LaTeX errors when creating PDF version.
  • PTXQC: R CMD check timed out

@batpigandme
Sorry, I couldn't find your email address... So, I just clicked "REQUEST EDIT ACCESS."

library(tidyverse)

res <- httr::GET("https://gist.githubusercontent.com/batpigandme/b59dca03bfb53b6e9a52332e306ff736/raw/bb05bfe98588d7a0194fd1fe59677d75efef3822/newlybroken_revdeps.md")

httr::content(res) %>% 
  stringr::str_split("\n") %>%
  unlist() %>%
  stringr::str_subset("^# ") %>%
  cat(sep = "\n")
#> # ActisoftR
#> # AneuFinder
#> # BloodCancerMultiOmics2017
#> # dartR
#> # derfinderPlot
#> # GenomicDataCommons
#> # ggmap
#> # ggridges
#> # ggtern
#> # gifski
#> # GOexpress
#> # MCbiclust
#> # mglR
#> # PTXQC
#> # RITAN
#> # scDD
#> # seqsetvis
#> # shadowtext
#> # tricolore
#> # tsbox

Created on 2018-09-26 by the reprex package (v0.2.1)

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 26, 2018

For ggtern, maybe the best way forward to maintain backwards compatibility with 3.0.0 is to reintroduce a variable called .element_tree and then assign the contents of that to ggplot_global$element_tree. In a later release, we can then remove .element_tree.

@nhamilton1980 Could you chime in?

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 26, 2018

And maybe the same should be done with .all_aesthetics, though I really don't know why people access that.

@yutannihilation

This comment has been minimized.

Member

yutannihilation commented Sep 26, 2018

For shadowtext, it's very strange... I'm sure the error occurs in this vignette: https://github.com/GuangchuangYu/shadowtext/blob/master/vignettes/shadowtext.Rmd and this acutally fails in and after devtools::check(), but I cannot reproduce the error in the fresh session...

reprex here: https://gist.github.com/yutannihilation/4f33a397a4fbedb8bf00dca9c6a0d12a

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 26, 2018

I suspect shadowtext is the same problem as ggridges. It uses an aesthetic called bgcolor.

@yutannihilation

This comment has been minimized.

Member

yutannihilation commented Sep 26, 2018

Ah, you are right. The error is basically this (though I'm not sure what happens when devtools::check() yet...):

scales::alpha(NULL, c(NA, NA))
#> Error in col[, rep(1, length(alpha)), drop = FALSE]: subscript out of bounds

Created on 2018-09-27 by the reprex package (v0.2.1)

@yutannihilation

This comment has been minimized.

Member

yutannihilation commented Sep 26, 2018

You are right. I got it; Since GeomShadowText's default_aes is determined on built, if I install the pre-compiled shadowtext binary via CRAN, bg.color stays as is. But, if it is built with ggplot2 v3.0.1, the default_aes no longer has bg.color since it is renamed to bg.colour. Accordingly, data$bg.color will be NULL, which cause the error above.

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 26, 2018

For the packages that fail on manual_scale(), they seem to be using code such as + scale_fill_manual(name = "abcd"):
https://github.com/SWRC/ActisoftR/blob/5df2f4255dcdf91935b90674709f591b347310eb/R/plot_Darwent.r#L134

I'm not sure what the purpose of such code is, but a workaround would be to set values = NULL:

library(ggplot2) # github master

ggplot(iris, aes(Sepal.Length)) + 
  geom_density() + scale_fill_manual(name = "abc")
#> Error in manual_scale(aesthetics, values, ...): argument "values" is missing, with no default

ggplot(iris, aes(Sepal.Length)) + 
  geom_density() + scale_fill_manual(name = "abc", values = NULL)

Created on 2018-09-26 by the reprex package (v0.2.0).

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 26, 2018

I've made a PR to fix ggtern: #2913 This should also fix BloodCancerMultiOmics2017 and tricolore.

I still need to investigate ggridges some more. Some things aren't working yet as expected. Once I've figured things out, I think it would be a good idea to write a blog post about best practices for non-standard aesthetics, so that we don't end up with a mess where everybody does something different. I'm glad that so far very few package developers seem to have tried to do this.

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 27, 2018

I have fixed ggridges and it is on its way to CRAN. In the mean time, here are notes on porting non-standard aesthetics from 3.0.0 to 3.0.1 and best practices for using non-standard aesthetics going forward.

Notes: ggplot2 has always supported both British and American spelling for the word "colour". However, there was no systematic support for alternative types of spelling in the ggplot2 internals. With the release of ggplot2 3.0.1, ggplot2 always uses British spelling internally, and any aesthetics that contain the word "color" are automatically translated into strings containing "colour". (For example, point_color is converted to point_colour.) This may affect package developers who have invented non-standard aesthetics. This code may behave differently in version 3.0.1 versus earlier versions.

First, if you have written a geom that defines British and American versions of your aesthetic, you may get warnings about duplicated aesthetics in ggplot2 3.0.1. Going forward, you must only list the British spelling in default_aes():

default_aes <- aes(point_colour = "red")

(If you list the American spelling, it is internally converted to British spelling.) For backwards compatibility with 3.0.0 or earlier, you can provide the American spelling as an optional aes:

optional_aes <- c("point_color")

Second, in the drawing code of your geom, the data frame will now hold a column named using the British spelling. However, for backwards compatibility with 3.0.0, you can write something lilke:

grid.gpar(col = data$point_color %||% data$point_colour)

This will use the American spelling if provided and the British spelling otherwise.
The operator %||% is defined in the rlang package.

The same construct can be used to make your geom use the standard colour aesthetic unless point_colour is provided:

grid.gpar(col = data$point_color %||% data$point_colour %||% data$colour)

For this to work, the default for point_colour needs to be NULL:

default_aes <- aes(point_colour = NULL)

Finally, it is a good idea to define default scales for any new aesthetics you define. For ggplot2 3.0.1 or later, you only need to define a scale for point_colour with the setting aesthetics = "point_colour", e.g.:

scale_point_colour_discrete <- function(...) scale_colour_hue(aesthetics = "point_colour", l = 40, ...)

For ggplot2 3.0.0 or earlier, you additionally need to define scale_point_color_discrete with aesthetics = "point_color", e.g.:

scale_point_color_discrete <- function(...) scale_colour_hue(aesthetics = "point_color", l = 40, ...)
@yutannihilation

This comment was marked as resolved.

Member

yutannihilation commented Sep 27, 2018

Thanks for the great note!

Minor fix. aes(point_colour = "NULL") should be aes(point_colour = NULL), right?

@clauswilke

This comment has been minimized.

Member

clauswilke commented Oct 1, 2018

@hadley Once we're done with NEWS, I think I have to hand this over to you. Most of the next steps need to be taken by the maintainer. And it's also not clear to me what the right contents of email.yml would be.

@hadley

This comment has been minimized.

Member

hadley commented Oct 1, 2018

@clauswilke would you mind directly reporting to AneuFinder? Then I think we can skip the emails.

@clauswilke

This comment has been minimized.

Member

clauswilke commented Oct 1, 2018

Already sent an email to the maintainer yesterday, so that’s done.

@hadley

This comment has been minimized.

Member

hadley commented Oct 1, 2018

Ok, let's plan for a release on Oct 10?

@clauswilke

This comment has been minimized.

Member

clauswilke commented Oct 1, 2018

Works for me.

@hadley

This comment has been minimized.

Member

hadley commented Oct 1, 2018

check_win_devel() revealed one NOTE:

Non-standard file/directory found at top level:
  'GOVERNANCE.md'

Would someone mind putting together a PR that adds it to .Rbuildignore?

batpigandme added a commit to batpigandme/ggplot2 that referenced this issue Oct 1, 2018

hadley added a commit that referenced this issue Oct 1, 2018

@hadley

This comment has been minimized.

Member

hadley commented Oct 23, 2018

Sorry for missing the release goal by two weeks. Before I re-kick off the process tonight/tomorrow, is there anything I should know?

@clauswilke

This comment has been minimized.

Member

clauswilke commented Oct 23, 2018

From my side, there's only PR #2942 that you could consider merging. You suggested that we may want to do this before the release, to close #2938.

And, apparently there are now Travis issues (#2957) that haven't been resolved, due to changes in vdiffr and devtools. Not sure whether they need to be resolved before a release, but until they're resolved any future merge will fail the Travis checks.

@hadley

This comment has been minimized.

Member

hadley commented Oct 23, 2018

Thanks @clauswilke — I've merged those PRs and added a GITHUB_PAT so the travis failures should hopefully be fixed soon.

I notice one missing piece is that we don't have an updated cran-comments.md. Since I emailed all the maintainers on Oct 1, I think I can just mention that, but in future we should probably be a bit more systematic. But just in case @topepo is going to re-run the revdeps, and I'll re-email everyone tomorrow.

@topepo

This comment has been minimized.

Member

topepo commented Oct 24, 2018

Can you add me to the project so I can start a new branch?

I'll rerun today with a longer timeout (see below for prelim results and DB is at https://www.dropbox.com/s/llilyu97detb5xv/data.sqlite?dl=0).

Spoiler alert:

We checked 2338 reverse dependencies (1971 from CRAN + 367 from BioConductor), comparing R CMD check results across CRAN and dev versions of this package.

  • We saw 3 new problems
  • We failed to check 53 packages

Issues with CRAN packages are summarised below.

New problems

(This reports the first line of each new failure)

  • FLightR
    checking examples ... ERROR

  • lime
    checking tests ...

  • postal
    checking tests ...

Failed to check

  • ALA4R (check timed out)
  • av (failed to install)
  • BACCT (failed to install)
  • bamdit (failed to install)
  • BayesRS (failed to install)
  • BMSC (failed to install)
  • BNSP (failed to install)
  • bsam (failed to install)
  • BTSPAS (failed to install)
  • choroplethr (failed to install)
  • classify (failed to install)
  • CoordinateCleaner (failed to install)
  • crmPack (failed to install)
  • DeLorean (failed to install)
  • dynr (failed to install)
  • ewoc (failed to install)
  • fdq (failed to install)
  • Fgmutils (failed to install)
  • fieldRS (failed to install)
  • fingerPro (failed to install)
  • gastempt (failed to install)
  • GENEAsphere (failed to install)
  • geofacet (failed to install)
  • ggstatsplot (check timed out)
  • GmAMisc (failed to install)
  • HTSSIP (check timed out)
  • ITGM (failed to install)
  • jarbes (failed to install)
  • JointAI (failed to install)
  • MetaStan (failed to install)
  • milr (check timed out)
  • morse (failed to install)
  • mwaved (failed to install)
  • onemap (check timed out)
  • pcaPA (failed to install)
  • phase1RMD (failed to install)
  • phylosim (check timed out)
  • pmc (check timed out)
  • ppcSpatial (failed to install)
  • RcmdrPlugin.FuzzyClust (check timed out)
  • rpanel (failed to install)
  • rstan (check timed out)
  • rstanarm (check timed out)
  • rsvg (failed to install)
  • seewave (failed to install)
  • SeqFeatR (failed to install)
  • sf (failed to install)
  • simmr (failed to install)
  • simulator (check timed out)
  • TeachingDemos (check timed out)
  • trialr (failed to install)
  • walker (check timed out)
  • zooaRchGUI (failed to install)
@batpigandme

This comment has been minimized.

Member

batpigandme commented Oct 24, 2018

Added you, @topepo! ✔️

@hadley

This comment has been minimized.

Member

hadley commented Oct 24, 2018

@topepo don't worry about running with a longer time out — given that >2000 packages didn't have problems I think we can assume we didn't accidentally introduce any regressions.

@thomasp85 can you please look into the lime issue?

Would someone else mind looking into the postal and FLightR issues?

@clauswilke

This comment has been minimized.

Member

clauswilke commented Oct 24, 2018

@topepo Do we have the updated problems.md file?

@hadley The packages lime and FLightR had issues the last time and I looked into it then. For lime, it looked like a network error. The relevant code needs to download data to run, and it worked just fine on my machine with ggplot2 master. Not sure why it reliably breaks in revdepcheck, though. For FLightR, the problem was with the changes to the google map API, specifically the new authentication requirement they added. Again, not sure why it shows up as a new problem in revdepcheck, it should break for old ggplot2 also.

topepo added a commit that referenced this issue Oct 24, 2018

@topepo

This comment has been minimized.

Member

topepo commented Oct 24, 2018

Do we have the updated problems.md file?

Just checked in.

@hadley

This comment has been minimized.

Member

hadley commented Oct 24, 2018

@clauswilke that's perfect - thanks!

hadley added a commit that referenced this issue Oct 24, 2018

@clauswilke

This comment has been minimized.

Member

clauswilke commented Oct 24, 2018

Ok, now that I have the new problems.md file, I looked more closely at lime and FLightR.

lime is the same version as in the previous revdepcheck, and the error looks similar as before (network error, fails to download a dataset it needs).

FLightR actually has released a new version that has addressed the ggmap issues. The problem there now also seems to be a network error (Error in socketConnection("localhost", port = port, server = TRUE, blocking = TRUE, : cannot open the connection). The examples work fine for me.

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Oct 24, 2018

You can certainly ignore lime

@hadley

This comment has been minimized.

Member

hadley commented Oct 25, 2018

It was just accepted by the CRAN bot! Thanks everyone for your hard work on this 🎉

@hadley

This comment has been minimized.

Member

hadley commented Oct 25, 2018

Any volunteers to work on the blog post? I'm currently attempting to get travis to build the docs automatically.

@clauswilke

This comment has been minimized.

Member

clauswilke commented Oct 25, 2018

Is there some convenient mechanism to work on it collaboratively? At a minimum I could provide some materials related to the parts I worked on.

@batpigandme

This comment has been minimized.

Member

batpigandme commented Oct 25, 2018

Oh, let me PR as far as I've gotten, and you can modify as you like

@clauswilke

This comment has been minimized.

Member

clauswilke commented Dec 10, 2018

I was just wondering whether we can close this issue and I noticed that we haven't done the last item of the check list: Add link to blog post in pkgdown news menu

@batpigandme Is this something you could do?

@batpigandme

This comment has been minimized.

Member

batpigandme commented Dec 10, 2018

@clauswilke Yup — not quite sure that's done, though, so I'm not gonna tick it off just yet #3033

@clauswilke clauswilke closed this Dec 13, 2018

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