ropensci / rsvg

SVG renderer for R based on librsvg2
Other
95 stars 1 forks source link

rsvg svg format has changed #40

Closed willgearty closed 9 months ago

willgearty commented 10 months ago

A recent update to the rsvg package has completely changed the format of the svg files that it creates. This has resulted in my package rphylopic completely breaking.

For example, if I use the following code with two different versions of rsvg, you get pretty dramatically different xml:

# create some svg
tmp <- tempfile()
svglite::svglite(tmp, width = 10, height = 7)
ggplot2::qplot(mpg, wt, data = mtcars, colour = factor(cyl))
dev.off()

rsvg_svg(tmp, "out.svg")

Based on bd1c0b0e7351d65d607a7dd01cb5f5c7025fb3c9: old.svg

Based on bb2086aab891eac730562b61b56d365dd69b500d: new.svg

Some immediate differences in the new version:

Tagging @pmur002 and @sjp since this is also relevant to their grImport2 package.

jeroen commented 10 months ago

~This is likely an upstream change in librsvg 2.50? I do know that all code was ported to rust around that release, and support for css was added, so perhaps that is the cause of the side effects?~

I don't think this is related to rsvg: https://github.com/ropensci/rsvg/issues/40#issuecomment-1742729507

willgearty commented 10 months ago

I should have mentioned that I'm on Windows. However, our package also appears to also be having issues on Mac (we test our Ubuntu build with a set librsvg version). I would guess it's all related to upstream changes, but I checked the librsvg changelog and nothing stood out to me. It could even be more upstream than that?

pmur002 commented 10 months ago

Maybe this is related to an update in Cairo version (I think that librsvg uses Cairo to generate SVG). Cairo 1.18.0 just dropped and CRAN might have instantly picked it up on some platforms. I know they are tracking the very latest versions of Cairo on their Debian platforms.

I will take a look at the changes necessary to 'grImport2'.

willgearty commented 10 months ago

That would be great, thanks @pmur002!

jeroen commented 10 months ago

@willgearty your example does not use rsvg or librsvg at all. Your svg is generated by the svglite package. I don't think that package uses cairo either. So I don't think the commits that you point to have anything to do with it?

Can you please try to specify which OS, which version of R and svglite that you use, and in which version the changes first appeared?

willgearty commented 10 months ago

@jeroen the only thing I changed for those two example files in my first post was the version of rsvg (denoted by the two commits which I installed via remotes::install_github). For both files, svglite was version 2.1.1, OS was Windows 11, and R was version 4.3.1. While the original svg file is created with svglite, the final Cairo svg files were created with rsvg; the last line of the example code calls rsvg::rsvg_svg (this example is straight from your readme).

Ultimately, I think @pmur002's changes to grImport2 will fix the problems in my rphylopic package, but the behavior of the rsvg package has definitely changed between those two commits (likely due to changes in upstream dependencies).

pmur002 commented 10 months ago

I have some fixes in a 'grImport2' version 0.3-0 on R-Forge.

According to my testing, 'rphylopic' works again as before with that update of 'grImport2', on a system that has ...

> rsvg::librsvg_version()
[1] "2.54.7"

> grSoftVersion()
                   cairo                  cairoFT                    pango 
                "1.18.0"          "2.13.2/2.14.2"                       "" 
                  libpng                     jpeg                  libtiff 
                "1.6.40"                    "6.2" "LIBTIFF, Version 4.5.1" 

@willgearty are you able to build and test that ?

I need to do some wider testing before I submit the new 'grImport2' version to CRAN.

willgearty commented 10 months ago

@pmur002 yes, everything seems to work with that new version of grImport2! Note that you might want to update the checkValidSVG function. It is still returning a warning for newer svgs.

willgearty commented 10 months ago

Oh, it appears you already knew that. We can just use warn = FALSE for now if necessary.

pmur002 commented 10 months ago

@willgearty Thanks for confirming. I will work on getting a new 'grImport2' version onto CRAN. It looks like the CRAN checks are OK for now - did you pick up the problem on your own testing?

jeroen commented 10 months ago

@willgearty I just pushed another few updates. Can you test again with the latest rsvg from https://ropensci.r-universe.dev/rsvg ?

willgearty commented 9 months ago

@pmur002 I was working on a new PR for our package when all of my unit tests (seemingly) randomly started failing locally. I then realized that the rsvg update seemed to be related (although it does ultimately seem like the cause was changes to librsvg and/or cairo). Please keep me updated on the CRAN progress. We'll have an updated version of rphylopic ready to get pushed to CRAN as soon as we can after grImport2 is updated on CRAN.

@jeroen Looks like a bunch of our test snapshots have changed, but I'm guessing that's due to more upstream changes in librsvg ([shakes fist]). As far as I can tell, the functionality of the rphylopic package still works after updating to rsvg v. 2.6.0. Please keep us updated on the CRAN progress of this new version of rsvg.

For reference, here is the output of the same code as the OP using rsvg 2.6.0: newer.svg

Thank you both for your help with this! We just had a publication come out for the package, and we would both have a breakdown if the package stopped working for users right now.

jeroen commented 9 months ago

I have just released rsvg 2.6.0 on cran. On MacOS it has librsvg 2.56.3 and on Windows it has librsvg 2.57.0. There was also a bug fixed for rsvg_svg() in this release: https://github.com/ropensci/rsvg/issues/41

Hopefully no more updates are needed for this package any time soon.

pmur002 commented 9 months ago

'grImport2' 0.3-0 is on its way to CRAN now.

willgearty commented 9 months ago

rphylopic 1.2.1 is up on CRAN now. Thank you both again for your help with this!