synfig
synfig copied to clipboard
synfig
fix: change convert.exe syntax to avoid clash in Windows
On Windows (I built with CMake) for some weird reason calling ImageMagick's convert calls Windows/System32/convert.exe . It's odd because my Path env var seems to list the mingw64 directory first, but a solution is using the 'magick' syntax, where you just call 'magick' to convert as per https://imagemagick.org/script/command-line-tools.php
The problem is widely known as per https://stackoverflow.com/questions/3060205/error-invalid-parameter-fom-imagemagick-convert-on-windows , I was getting the same 'Invalid Parameter' error
The patch uses 'magick convert' (legacy syntax), instead of 'magick' for compatibility with ImageMagick 6, since many distros don't have official packages for ImageMagick 7, despite the release being over 5 years old:
- https://packages.debian.org/search?searchon=sourcenames&keywords=imagemagick
- https://packages.fedoraproject.org/pkgs/ImageMagick/ImageMagick/
- https://packages.ubuntu.com/search?keywords=imagemagick
The reason is that v7's syntax is not fully compatible with v6 but it our syntax seems safe
TL;DR: Instead of using ImageMagick with 'convert' use 'magick convert' since Win has another 'convert' program
It's odd because my Path env var seems to list the mingw64 directory first,
Can you describe your build environment/process? I also experienced this issue before, but I don't remember how to reproduce it.
but a solution is using the 'magick' syntax, where you just call 'magick' to convert as per
Yes, I was also thinking about the same solution, but we can't use it because some OSes (like Debian 10) are still using ImageMagick 6, so they don't have the magick binary.
https://pkgs.org/search/?q=imagemagick
Can you describe your build environment/process?
- I'm using a Windows 10 64-bit Home ( version 21H2 ), on an x64-based processor
- I'm compiling under MSYS2 MINGW64 ( version 20221028 )
- I'm building with CMake with type Release since Debug exports too many symbols
- printing 'getenv( "Path" )' from RunPipeWin32::open() outputs
C:\Users\coz\Desktop\Agames\tools\msys64\home\coz\synfig\cmake-build-msys\output\Release\bin;C:\Users\coz\Desktop\Agames\tools\msys64\mingw64\bin;C:\Users\coz\Desktop\Agames\tools\msys64\usr\local\bin;C:\Users\coz\Desktop\Agames\tools\msys64\usr\bin;C:\Users\coz\Desktop\Agames\tools\msys64\usr\bin;C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Users\coz\Desktop\Agames\tools\msys64\usr\bin\site_perl;C:\Users\coz\Desktop\Agames\tools\msys64\usr\bin\vendor_perl;C:\Users\coz\Desktop\Agames\tools\msys64\usr\bin\core_perl - '$whereis convert' outputs
convert: /mingw64/bin/convert.exe /c/Windows/System32/convert.exe
And that is it, I'm not sure if there's anything else that can be relevant
Yes, I was also thinking about the same solution, but we can't use it because some OSes (like Debian 10) are still using ImageMagick 6, so they don't have the magick binary.
Aaaah, I thought the 'legacy' syntax allowed near-identical use with v6
Well, I found the reason of the issue As per https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa regardless of what you have in PATH dearest windows looks in the 32-bit system directory first when running CreateProcessW in os.cpp .
The only sensible solution then is to pass the full path to the convert utility
Well, I found the reason of the issue As per https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa regardless of what you have in PATH dearest windows looks in the 32-bit system directory first when running CreateProcessW in os.cpp .
Exactly!
The only sensible solution then is to pass the full path to the convert utility
Agree.
As per https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa regardless of what you have in PATH dearest windows looks in the 32-bit system directory first when running CreateProcessW in os.cpp . On 64bit systems, System32 contains 64bit dlls, SysWOW64 contains the 32bit ones ;)
The implied precedence for system directories is due to the risk of injection of dlls which would be loaded first from the directory of the application. The same concern was affecting ffmpeg on Windows, you could get inspiration from the mod_ffmpeg module to deal with this.
Other fact that I noticed from 1.4.3 and 1.5.1: there is no trace of "convert" or "magick" binaries while "convert.exe" was present in 1.2.2
mod_ffmpeg works by relying on the packager; it assumes ffmpeg.exe will always be in the same directory as synfig.exe, but that fails on the CygWin / MinGW environment
I could implement a fix for imagemagick (traverse PATH looking for convert) but given the inconsistencies with other external dependencies like ffmpeg it feels like a hack. Instead I'm going to work on Synfig by starting from the base. Since that needs majority agreement, I made a new post in the forums. I hope that Konstantin can also chime in too for those general decisions
Other fact that I noticed from 1.4.3 and 1.5.1: there is no trace of "convert" or "magick" binaries while "convert.exe" was present in 1.2.2
@morevnaproject
Since my 'fix' is not convenient at this time, I have no problem if the pull request is closed; an issue could be created for the convert.exe clash instead if desired. That being said, if it would be better to leave this pull request open as a reminder that the clash still happens, that's also fine with me. I'm not really sure what would fit the devs workflow best.
but a solution is using the 'magick' syntax, where you just call 'magick' to convert as per
Yes, I was also thinking about the same solution, but we can't use it because some OSes (like Debian 10) are still using ImageMagick 6, so they don't have the
magickbinary.
What about test "magick" first? If it doesn't exist, we try traditional "convert" then.
@coz-eduardo-hernandez @ice0 what do you think about this: https://github.com/rodolforg/synfig/commit/d78e4f51d29adc634751b71fda97b131d97a24e7 ?