-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
ImageMagick: switch to CMake, migrate to Conan v2 #21699
base: master
Are you sure you want to change the base?
Conversation
@Cyriuz Could you please sign the CLA for this PR? |
This comment has been minimized.
This comment has been minimized.
0927c79
to
ef9ff98
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This fixes the previously broken Windows build and adds a bunch of new possible dependencies.
4136f93
to
d4f6254
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ❌Failure in build 11 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping See details:Failure in build 11 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Hey there, small feedback on the relevance of this PR: libvips requires imagemagick for working with .bmp files ( libvips/libvips#1528 ). If I understand the changes correctly, this PR would enable imagemagick for windows? I would very much welcome this. |
@valgur What is the state of this? I tried using it with the newest Imagemagick (7.1.1-32) and it worked? I had to make some changes though: I changed the freetype version to 2.15.0 and disabled fftw and openmp since they did not work. In the test_package the identify test does not work, as it cannot find it, and the check for (Built on Windows mvsc) |
@Julianiolo Thanks for testing the recipe. That's good to know! However, the custom build system included from the forked repo is a too significant change to be acceptable on CCI, I'm afraid. It should really be merged upstream instead, ultimately. For CCI, a more realistic approach would be to substitute the custom "configure.exe" executable on Windows with a simpler workaround, I think. |
Yeah I guess the build situation with imagemagick is a little .... unfortunate at the moment. About an configure.exe workaround: I guess that could work, but that would probably be the same deal as with cmake now. I'm not sure on how this is handled best. Maybe using the cmake solution isn't that bad of an idea?, it does not seem they will adopt more portable build systems any time soon ... |
I guess it would maybe be possible to use msys2/mingw gcc, but I don't know how conan would/can handle that, when building with msvc... |
The big problem is that they won't accept any new build system unless it is used by the community and "proven" to work, so at least when I tried to get this into CCI it was with the intent to do just that and then possibly get it merged upstream later. But yes it would be quite some code to maintain on the recipe level unfortunately. |
Thanks for working on this! I've been using this recipe locally and it's working fairly well. I've encountered two small issues though:
Since I copied the recipe locally I can just work around this issues easily, but I figured I'd let you know :) |
I patched out the MFC dependency for configure.exe on Windows at #18623, so that version should support all platforms now. |
Continues from #10003 by @Cyriuz.
It merges the build scripts from the latest commit at https://github.com/Cyriuz/ImageMagick into the official source archive.
I also exposed all available options and added support for all dependencies available on CCI.