Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

use Librsvg_jll pixbuf loader (fixes #604) #605

Merged
merged 11 commits into from
Jan 17, 2022
Merged

use Librsvg_jll pixbuf loader (fixes #604) #605

merged 11 commits into from
Jan 17, 2022

Conversation

jwahlstrand
Copy link
Contributor

This uses Librsvg's pixbuf loader so that we can load SVG icons and render SVG through GdkPixbuf. It copies the loaders from gdk_pixbuf_jll and Librsbg_jll into a directory and runs gdk_pixbuf_query_loaders() on it. Thanks to @giordano for help with that last part.

Questions for the more highly educated than me: would it be better to create symbolic links rather than copy? What happens if Librsvg_jll or gdk_pixbuf_jll are updated? Is the MutableArtifacts.toml cache automatically regenerated after an update or should we handle that somehow?

Probably also fixes #525.

@tknopp
Copy link
Collaborator

tknopp commented Jan 2, 2022

errors on Julia 1.3 due to some new readdir functionality being used. Can this be implemented in a 1.3 compatible fashion?

@jwahlstrand
Copy link
Contributor Author

Yes, I'll fix that. I'll investigate the issue with Windows.

@giordano
Copy link
Contributor

giordano commented Jan 2, 2022

Consider simply dropping support for Julia v1.5-, now that v1.6 is the LTS?

@tknopp
Copy link
Collaborator

tknopp commented Jan 2, 2022

I am fine with that

@jwahlstrand
Copy link
Contributor Author

I can't figure out why Windows is failing, but it's occuring in readchomp(addenv(...)). I'll be able to return to this later.

@codecov
Copy link

codecov bot commented Jan 8, 2022

Codecov Report

Merging #605 (37e4d63) into master (ceeb0b1) will increase coverage by 0.82%.
The diff coverage is 94.44%.

❗ Current head 37e4d63 differs from pull request most recent head 6f2474b. Consider uploading reports for the commit 6f2474b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
+ Coverage   56.80%   57.62%   +0.82%     
==========================================
  Files          32       32              
  Lines        2669     2714      +45     
==========================================
+ Hits         1516     1564      +48     
+ Misses       1153     1150       -3     
Impacted Files Coverage Δ
src/Gtk.jl 91.07% <94.44%> (+5.88%) ⬆️
src/GLib/GLib.jl 77.27% <0.00%> (ø)
src/GLib/signals.jl 75.91% <0.00%> (+0.91%) ⬆️
src/events.jl 46.93% <0.00%> (+2.04%) ⬆️
src/base.jl 79.74% <0.00%> (+4.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ceeb0b1...6f2474b. Read the comment docs.

@jwahlstrand
Copy link
Contributor Author

Not sure what to make of these CI failures.

The previous error on Windows went away by switching to withenv(), but GtkSpinner still doesn't work on Windows (it does on Linux and Mac). Now I'm seeing some permission errors (g_module_open() failed for [artifact_dir\*.dll]: Access is denied). However since my Windows computer is pretty broken it would be helpful if someone else could test this. @BioTurboNick ?

@BioTurboNick
Copy link
Contributor

BioTurboNick commented Jan 8, 2022

@jwahlstrand Sure, checked it out.

Note - MutableArtifacts.toml has to be deleted to get the initializing code to rerun.

EDIT: Nevermind that deleted bit, learning about backtick Cmd objects now.

The files are successfully copied, but the problem appears to exist when gdk-pixbuf-query-loaders is run.

@BioTurboNick
Copy link
Contributor

BioTurboNick commented Jan 8, 2022

Okay, here's something.

Other artifact directories are created with Full Control access permissions to the current user, and no permissions to Everyone.

Meanwhile, the special shared artifact directory created here has limited permissions for the current user (which is probably the most important part) and also adds Everyone.

image

EDIT to add:

And the culprit appears to be this line in Pkg.jl, create_artifact():

chmod(new_path, filemode(dirname(new_path)))

@BioTurboNick
Copy link
Contributor

BioTurboNick commented Jan 8, 2022

What I posted over in the Julia issue may be the problem. The original files have "Read & execute" permissions in the original folder, but that permission gets removed by Pkg in the new artifact folder.

@BioTurboNick
Copy link
Contributor

BioTurboNick commented Jan 8, 2022

Ah, it works!

If I insert a pause between creating the shared artifact directory and consuming the libraries, set permissions to full control and override child permissions, then allow it to continue, I have a functioning spinner.

image

@BioTurboNick
Copy link
Contributor

@jwahlstrand - sorry I think I wasn't clear. I didn't mean to sleep the thread, I meant that if you manually set the permissions before it continues, it will then work.

@jwahlstrand
Copy link
Contributor Author

Oops, OK I'll remove that.

@jwahlstrand
Copy link
Contributor Author

It now works on my Windows computer (both 1.6 and 1.7). But in CI there are still failures for Windows: for Julia 1 (1.7) and nightly it's unable to find Librsvg_jll.libpixbufloader_svg.

@jwahlstrand
Copy link
Contributor Author

Ahh, I didn't notice at first that the issue is only on Windows x86. Now it's clear why -- Librsvg_jll is not built for that arch.

src/Gtk.jl Outdated Show resolved Hide resolved
@giordano
Copy link
Contributor

Now it's clear why -- Librsvg_jll is not built for that arch.

Because Rust toolchain for i686-w64-mingw32 is broken when using SJLJ unwinding (which is what we do): rust-lang/rust#79609. Judging by the comments of the developers, it's unlikely they'll fix it

@tknopp
Copy link
Collaborator

tknopp commented Jan 16, 2022

ready? CI is passing.

@jwahlstrand
Copy link
Contributor Author

If this last commit passes, it's ready.

@jwahlstrand
Copy link
Contributor Author

I'll keep working on this. In the meantime I wouldn't let this delay a release.

@jwahlstrand
Copy link
Contributor Author

OK, the failure earlier must have been a fluke...I think this is good to go now.

@tknopp tknopp merged commit 34b1a1a into JuliaGraphics:master Jan 17, 2022
@jwahlstrand jwahlstrand deleted the svg branch January 17, 2022 19:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot display SVG files, GtkSpinner not displayed
4 participants