Skip to content

Massive clean up of PGalias.pm and PGresource.pm.#1046

Merged
Alex-Jordan merged 2 commits intoopenwebwork:developfrom
drgrice1:pgalias-rewrite
Apr 30, 2024
Merged

Massive clean up of PGalias.pm and PGresource.pm.#1046
Alex-Jordan merged 2 commits intoopenwebwork:developfrom
drgrice1:pgalias-rewrite

Conversation

@drgrice1
Copy link
Copy Markdown
Member

@drgrice1 drgrice1 commented Mar 26, 2024

Rewrite the PGalias.pm and PGresource.pm files so that the implemenation makes much more sense and doesn't have the massive kludge that they were.

The behavior is the same as it was. The structure of the data stored in a PGresource object is different (all of the things that were only used internally by PGalias are no longer there), and a minor change to webwork2 is needed to accommodate for that change.

There is one change of note. This switches PGalias.pm from using the "gif2png" external program to using the "convert" external program to convert gif files to png files. Note that the "gif2png" external program is "/usr/bin/giftopnm" by default. Of course "convert" is imagemagick's convert utility. This works just as well (if not better). Note that an animated gif is converted to the first frame (same as before).

Note that "csv" files are also added to the list of allowed auxiliary resources. At this point this is not used, but the rserve code needs to be updated to use PGalias. So that is what this is for. I see no reason that a csv file should not be allowed in any case. An author may want to provide a static csv data file with a problem.

The copy of the findMacroFile that was in PGalias.pm has been deleted. The PG.pl findMacroFile calls the method in PGloadfiles.pm instead. Note that the copy in PGalias.pm was not entirely functional to begin with. Also note that the version in PG.pl is only called by the deprecated answerDiscussion.pl macro. The only other places where this method is called is by PGloadfiles.pm internally (for actually loading macro files).

The findAppletCodebase stub in PGalias.pm was also deleted. The only place that is called is by the findAppletCodebase method in PG.pl. So that method is now the stub directly.

Comment thread lib/PGresource.pm Outdated
@drgrice1 drgrice1 force-pushed the pgalias-rewrite branch 2 times, most recently from a7d4c27 to 1808683 Compare March 27, 2024 22:02
@pstaabp
Copy link
Copy Markdown
Member

pstaabp commented Mar 30, 2024

While testing, I was looking for different resources to make sure everything is loading correctly.

This problem: Contrib/USM/setCombinatorial_Game_Theory/flower_with_2_leaves_and_3_petals.pg is fine in the browser, but gives an error on hardcopy:

! LaTeX Error: Unknown graphics extension: .svg.

I'm guessing this needs to be converted to PDF for latex hardcopy.

Since has the same behavior on both develop and this branch, this may not be relevant for this, but should be fixed.

@drgrice1
Copy link
Copy Markdown
Member Author

Actually, since the convert_file_to_png_for_tex method was switched to using imagemagick's convert, that method will now also work for svgs. So I updated PGalias.pm to make that conversion.

@pstaabp
Copy link
Copy Markdown
Member

pstaabp commented Apr 3, 2024

I'm not seeing the same error, but convert from SVG to PNG on MacOS is not working right. Trying to track down a fix.

@pstaabp
Copy link
Copy Markdown
Member

pstaabp commented Apr 3, 2024

Just tested on ubuntu and the image isn't being converted. Just tested another svg file and the same result.

@drgrice1
Copy link
Copy Markdown
Member Author

drgrice1 commented Apr 3, 2024

What svg are you using? Maybe it is not a valid svg file to being with.

@pstaabp
Copy link
Copy Markdown
Member

pstaabp commented Apr 3, 2024

I tested the one in the problem cited above and also tested the pi.svg file in htdocs/images. Both resulted in blank images.

@drgrice1
Copy link
Copy Markdown
Member Author

drgrice1 commented Apr 3, 2024

On Ubuntu those files convert fine for me. Don't know why it wouldn't work for you. Have you tested from the command line?

@Alex-Jordan
Copy link
Copy Markdown
Contributor

Trying to convert the flower svg to png using convert on Mac. The ImageMagick version is 7.1.1-28.

If I do convert flower_with_2_leaves_and_3_petals.svg flower_with_2_leaves_and_3_petals.png I just get an all-white png. If I do convert -background none flower_with_2_leaves_and_3_petals.svg flower_with_2_leaves_and_3_petals.png, I get the following:

flower_with_2_leaves_and_3_petals

Maybe you can't see it in GitHub, but if you click to see that image, I do see two white dots surrounded by no background. Those dots correspond to dots in the original svg, but the rest of the image is missing.

Over on my development server (Oracle 8.7) the ImageMagick version is 6.9.12-77. There, if I run convert flower_with_2_leaves_and_3_petals.svg flower_with_2_leaves_and_3_petals.png I get a png that is faithful to the original svg.

@somiaj
Copy link
Copy Markdown
Contributor

somiaj commented Apr 5, 2024

I know the switch to imagemagick 7 has changed lots of things. My very limited understanding is imagemagick 7 now uses a single binary magick which we need to call for everything (no longer is split into multiple binaries like convert, mogrify, etc). I know Debian is keeping imagemagick 6 due to these breaking changes.

We may need to detect for (or have a setting for) systems that use imagemagick 7, and adjust commands appropriately.

@somiaj
Copy link
Copy Markdown
Contributor

somiaj commented Apr 5, 2024

@Alex-Jordan, does running magick image.svg image.png work on the system with imagemagick 7?

https://imagemagick.org/script/convert.php

With svg files we may need to also specify the size of the image?

@Alex-Jordan
Copy link
Copy Markdown
Contributor

Alex-Jordan commented Apr 5, 2024 via email

@somiaj
Copy link
Copy Markdown
Contributor

somiaj commented Apr 5, 2024

Lost a comment in my ninja edit, does adding -size 250x250 or maybe -size 250x250 canvas:none help?

@Alex-Jordan
Copy link
Copy Markdown
Contributor

Alex-Jordan commented Apr 5, 2024 via email

@somiaj
Copy link
Copy Markdown
Contributor

somiaj commented Apr 5, 2024

Looking a bit deeper, it seems that imagemagick 7 uses one of {MSVG|InkScape|RSVG} to render svgs, and the one being used is dependent on build time options. You should be able to add a -verbose flag to see what render is being used, but it could be some of the validation issues you are found are relevant depending on which svg renderer that imagemagick binary is using.

@Alex-Jordan
Copy link
Copy Markdown
Contributor

I get this:

alex.jordan@CASS317M4138 Downloads % magick -verbose flower_with_2_leaves_and_3_petals.svg flower_with_2_leaves_and_3_petals.png
'inkscape' '/var/folders/kz/9h51ybtj493_4f_czy1ty8x40000gr/T/magick-GxIzdR_BiIYv8dSKsgxcJVbWMnJrt4KM' --export-filename='/var/folders/kz/9h51ybtj493_4f_czy1ty8x40000gr/T/magick-sd_fS1wJTQh8LO0S7wHY12XFGgCCX5RV.png' --export-dpi='96' --export-background='rgb(100%,100%,100%)' --export-background-opacity='1' > '/var/folders/kz/9h51ybtj493_4f_czy1ty8x40000gr/T/magick-5lWLmhFlBb_GaYYLG3OUVqAsjLvf3L8h' 2>&1
mvg:/var/folders/kz/9h51ybtj493_4f_czy1ty8x40000gr/T/magick-Hzzp5ISuxac-a_f6vZARCnDQ0IAUJryA=>/var/folders/kz/9h51ybtj493_4f_czy1ty8x40000gr/T/magick-Hzzp5ISuxac-a_f6vZARCnDQ0IAUJryA MVG 564x417 564x417+0+0 16-bit sRGB 5214B 0.200u 0:00.127
flower_with_2_leaves_and_3_petals.svg SVG 564x417 564x417+0+0 16-bit sRGB 5214B 0.000u 0:00.000
flower_with_2_leaves_and_3_petals.svg=>flower_with_2_leaves_and_3_petals.png SVG 564x417 564x417+0+0 16-bit sRGB 5214B 0.020u 0:00.010

@somiaj
Copy link
Copy Markdown
Contributor

somiaj commented Apr 5, 2024

Well it does appear that binary is using inkscape, which is one of the validation errors you got. If you test with an .svg that passes the inkscape validation, things work?

@Alex-Jordan
Copy link
Copy Markdown
Contributor

I removed all inkscape- and rdf-related attributes and elements from the svg, and it still did not validate, citing the inkscape erorr. The validator points to a sodpodi attribute for that error, so I removed all sodipodi-related attributes. Then it validated. And I confirmed I can still open that SVG in Firefox and it looks fine with all those things removed.

But still, when I run convert, I get a blank png. Using magick does not change that.

@drgrice1
Copy link
Copy Markdown
Member Author

drgrice1 commented Apr 5, 2024

@somiaj: There will be no need for a switch to detect whether to use convert or magick. The system administrator would just need to set the correct executable path in the settings.

In any case, at least LaTeX will succeed in generating a PDF with this change if an svg image is in the probelm (even if a later version of imagemagick isn't working right). With the develop branch, it just fails. If someone using imagemagick 7 can investigate and see what is wrong there, then we can look into a fix.

@pstaabp
Copy link
Copy Markdown
Member

pstaabp commented Apr 6, 2024

I was looking into this a few days ago and see the same things as @Alex-Jordan. One thing that worked for me was to use the rvsg library to do the conversion.

I can't seem to find the site that I found this, but in the delegates.xml file (on my Mac it was /opt/homebrew/etc/ImageMagick-7/delegates.xml) I added the line:

<delegate decode="rsvg" command="&quot;rsvg-convert&quot; -o &quot;%o&quot; &quot;%i&quot;"/>

and then on the convert, I prefixed RSVG: to the first image, like

convert -verbose RSVG:flower_with_2_leaves_and_3_petals.svg test.png

and the image converted correctly. You can also use

rsvg-convert flower_with_2_leaves_and_3_petals.svg -o test2.png

I may have needed to install the rsvg code instead. I didn't get to the point to get WW/PG to do use this yet.

@drgrice1
Copy link
Copy Markdown
Member Author

drgrice1 commented Apr 7, 2024

<delegate decode="rsvg" command="&quot;rsvg-convert&quot; -o &quot;%o&quot; &quot;%i&quot;"/>

That is interesting. On Ubuntu with ImageMagick 6, the file /etc/ImageMagick-6/delegates.xml has the line <delegate decode="svg" command="&quot;rsvg-convert&quot; -o &quot;%o&quot; &quot;%i&quot;"/>. I suspect if you use this instead, then you will not need to prefix the file name with RSVG:.

@drgrice1
Copy link
Copy Markdown
Member Author

drgrice1 commented Apr 7, 2024

The file also has the line <delegate decode="svg:decode" stealth="True" command="&quot;inkscape&quot; &quot;%s&quot; --export-filename=&quot;%s&quot; --export-dpi=&quot;%s&quot; --export-background=&quot;%s&quot; --export-background-opacity=&quot;%s&quot; &gt; &quot;%s&quot; 2&gt;&amp;1"/> which may be related to the inkscape issues.

@drgrice1
Copy link
Copy Markdown
Member Author

drgrice1 commented Apr 7, 2024

I also don't think that the svg conversion issues should be a stopper for this pull request. As I mentioned the current status of hardcopy generation with an svg is failure anyway. The svg conversion was added as an afterthought to this pull request, and the other aspects of this are more important than that.

@drgrice1
Copy link
Copy Markdown
Member Author

I did some more testing on this and I see that ImageMagick does use the lines in the delegates.xml file for determining how to convert a file.

I discovered that if I have the version of inkscape installed from the Ubuntu snap, then calling convert flower_with_2_leaves_and_3_petals.svg output.png results in the essentially completely transparent png. However, if I have the version of inkscape in the apt repository installed (which happens to be the same inkscape version number at this time for Ubuntu 22.04), then the resulting png image is correct. If I change the previously mentioned lines in delegates.xml to <delegate decode="rsvg" command="&quot;rsvg-convert&quot; -o &quot;%o&quot; &quot;%i&quot;"/> and <delegate decode="rsvg:decode" stealth="True" command="&quot;inkscape&quot; &quot;%s&quot; --export-filename=&quot;%s&quot; --export-dpi=&quot;%s&quot; --export-background=&quot;%s&quot; --export-background-opacity=&quot;%s&quot; &gt; &quot;%s&quot; 2&gt;&amp;1"/> (so just changing "svg" to "rsvg"), then it doesn't matter which inkscape is installed, the conversion does not work right. However, in that case if you add the rsvg: prefix to the svg filename, then it works (assuming you have rsvg-decode installed).

Note that this is all with ImageMagick version 6.9.11.

So the conclusion is that the conversion issues are not related to the version of ImageMagick, but are related to the set up and the availability of inkscape, and it being configured correctly in the delegates.xml file. In the delegates file, it should not have "decode=rsvg", it should be "delegates=svg". Note that snap version of inkscape can not even open a file from the command line, and that is the reason things fail with that. That is all just the stupidity of snaps.

If you are using ImageMagick version 7, then you just set the external program to be /usr/bin/magick instead of /usr/bin/convert. In either case, you need to have your delegates.xml file set up correctly, and the needed packages installed (and no snap inkscape on Ubuntu). Then everything will work.

So this is not an issue with how this is set up with the code in PG as implemented in this pull request. It is an issue with documenting how to set up ImageMagick and inkscape on systems so that it will work. On Ubuntu it is as simple as installing inkscape from the apt repository instead of the snap. It sounds like on OSX the delegates file will need to be amended.

@drgrice1
Copy link
Copy Markdown
Member Author

By the way, note that this does depend on the svg too. Regardless of which inkscape is installed, the pi.svg image in the webwork2 htdocs/images directory works fine on Ubuntu. So ImageMagick is detecting if inkscape is actually needed or not, most likely by examining the xmlns attributes.

@drgrice1 drgrice1 force-pushed the pgalias-rewrite branch 2 times, most recently from fb8050c to b13533d Compare April 17, 2024 18:48
Rewrite the PGalias.pm and PGresource.pm files so that the implemenation
makes much more sense and doesn't have the massive kludge that they
were.

The behavior is the same as it was.  The structure of the data stored in
a PGresource object is different (all of the things that were only used
internally by PGalias are no longer there), and a minor change to
webwork2 is needed to accomodate for that cchange.

There is one change of note.  This switches PGalias.pm from using the
"gif2png" external program to using the "convert" external program to
convert gif files to png files.  Note that the "giv2png" external
program is "/usr/bin/giftopnm" by default.  Of course "convert" is
imagemagick's convert utility. This works just as well (if not better).
Note that an animated gif is converted to the first frame (same as
before).

Note that "csv" files are also added to the list of allowed auxiliary
resources. At this point this is not used, but the rserve code needs to
be updated to use PGalias.  So that is what this is for. I see no reason
that a csv file should not be allowed in any case.  An author may want
to provide a static csv data file with a problem.

The copy of the `findMacroFile` that was in PGalias.pm has been deleted.
The PG.pl `findMacroFile` calls the method in PGloadfiles.pm instead.
Note that the copy in PGalias.pm was not entirely functional to begin
with.  Also note that the version in PG.pl is only called by the
deprecated answerDiscussion.pl macro.  The only other places where this
method is called is by PGloadfiles.pm internally (for actually loading
macro files).

The `findAppletCodebase` stub in PGalias.pm was also deleted.  The only
place that is called is by the `findAppletCodebase` method in PG.pl.  So
that method is now the stub directly.
The existing `convert_file_to_png_for_tex` already works for that.
@Alex-Jordan
Copy link
Copy Markdown
Contributor

Merging this now, and the corresponding webwork2 PR. I'm unsure where to make note of the inkscape issue so that it doesn't get lost.

@Alex-Jordan Alex-Jordan merged commit 14bf98e into openwebwork:develop Apr 30, 2024
@drgrice1
Copy link
Copy Markdown
Member Author

I made a note in the release notes already. It is not complete as to what needs to be done on systems other than Ubuntu though.

@drgrice1 drgrice1 deleted the pgalias-rewrite branch May 17, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants