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

Should ggsave use ragg if available? #4086

Closed
thomasp85 opened this issue Jun 24, 2020 · 9 comments
Closed

Should ggsave use ragg if available? #4086

thomasp85 opened this issue Jun 24, 2020 · 9 comments

Comments

@thomasp85
Copy link
Member

Just a point for discussion. I don't think ggplot2 should depend on it (at least not yet), since it is not critical to its use, but should we check if it is installed and use it if it is for png, tiff, and jpeg files?

@yutannihilation
Copy link
Member

I think yes, considering the default Windows graphic device has problems.

c.f. #4040

@thomasp85
Copy link
Member Author

I think the next release of ragg with full support of rtl and bidirectional text will make such a transition even more obvious. Should we even import ragg to make sure it is present? Thoughts?

@clauswilke
Copy link
Member

I think the only argument against importing it would be if there are systems or old setups where ragg is not available. If that's not the case, then I'd support importing ragg. I'm also planning to use it as the default null device in cowplot.

@yutannihilation
Copy link
Member

yutannihilation commented Sep 8, 2020

Just a quick note. Since systemfonts seems not available on R <=3.3, we cannot require ragg on these versions (as it imports systemfonts). R 3.3 is still within the tidyverse's R version support policy, so we cannot import ragg until R 4.1 is released, or systemfonts will be available again (I believe it will).

@thomasp85
Copy link
Member Author

In what way is it not available? As a binary or completely? There is nothing in ragg preventing it from being used on 3.3 and earlier, but CRAN will not build binaries on old versions

@yutannihilation
Copy link
Member

yutannihilation commented Sep 8, 2020

As probably you already notice (so you disabled GitHub Actions CI check on these versions, right...?), the compilation on R <=3.3 fails with the following error. This error is copied from https://github.com/r-lib/systemfonts/runs/1031786802#step:10:19, but I kept seeing this error on the ggplot2's CI until #4192. Should I file an issue on systemfont's repo?

 * installing *source* package ‘systemfonts’ ...
Found pkg-config cflags and libs!
Using PKG_CFLAGS=-I/usr/include/freetype2
Using PKG_LIBS=-lfontconfig -lfreetype
** libs
rm -f systemfonts.so caches.o cpp11.o dev_metrics.o font_matching.o font_registry.o ft_cache.o string_shape.o font_metrics.o string_metrics.o emoji.o cache_store.o init.o unix/FontManagerLinux.o
g++ -std=c++11 -I/opt/R/3.3.3/lib/R/include -DNDEBUG -I/usr/include/freetype2 -I/usr/local/include -I"/home/runner/work/_temp/Library/cpp11/include"   -fpic  -g -O2 -c caches.cpp -o caches.o
In file included from caches.cpp:1:0:
caches.h:19:18: error: variable or field ‘init_caches’ declared void
 void init_caches(DllInfo* dll);
                  ^
caches.h:19:18: error: ‘DllInfo’ was not declared in this scope
caches.h:19:27: error: ‘dll’ was not declared in this scope
 void init_caches(DllInfo* dll);
                           ^
caches.h:21:20: error: variable or field ‘unload_caches’ declared void
 void unload_caches(DllInfo* dll);
                    ^
caches.h:21:20: error: ‘DllInfo’ was not declared in this scope
caches.h:21:29: error: ‘dll’ was not declared in this scope
 void unload_caches(DllInfo* dll);
                             ^
caches.cpp:33:18: error: variable or field ‘init_caches’ declared void
 void init_caches(DllInfo* dll) {
                  ^
caches.cpp:33:18: error: ‘DllInfo’ was not declared in this scope
caches.cpp:33:27: error: ‘dll’ was not declared in this scope
 void init_caches(DllInfo* dll) {
                           ^
caches.cpp:41:20: error: variable or field ‘unload_caches’ declared void
 void unload_caches(DllInfo* dll) {
                    ^
caches.cpp:41:20: error: ‘DllInfo’ was not declared in this scope
caches.cpp:41:29: error: ‘dll’ was not declared in this scope
 void unload_caches(DllInfo* dll) {
                             ^
/opt/R/3.3.3/lib/R/etc/Makeconf:141: recipe for target 'caches.o' failed
make: *** [caches.o] Error 1
ERROR: compilation failed for package ‘systemfonts’
* removing ‘/tmp/RtmpXJbmnR/Rinst2e9d177b8432/systemfonts’
      -----------------------------------
ERROR: package installation failed
##[error]Error in proc$get_built_file() : Build process failed
Calls: <Anonymous> ... build_package -> with_envvar -> force -> <Anonymous>
Execution halted
##[error]Process completed with exit code 1.

@thomasp85
Copy link
Member Author

Sorry, I was looking at ragg when I wrote the last message. Shouldn't be any problem to make it work on 3.3 again

@yutannihilation
Copy link
Member

Thanks. My only concern is that cpp11 seems to have some problems about testing on R 3.3. Not sure if this is some actual problem or not, though...

https://github.com/r-lib/cpp11/blob/915c5dce916bbfb65dff92ace5fda13cf6e47d73/.github/workflows/R-CMD-check.yaml#L37

@yutannihilation
Copy link
Member

I believe this is already addressed by #4388.

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

No branches or pull requests

3 participants