search

dpage / winpgbuild

A repo containing Github actions for building PostgreSQL and it's dependencies on Windows
1 stars 2 forks source link

libxslt: use cmake #3

Closed anarazel closed 2 months ago

anarazel commented 2 months ago

Given that libxml is already built with cmake and that the cmake based build generates pkg-config files...

dpage commented 2 months ago

I'm on the fence about this change (although other-Dave committed it already). The original intent of this project was to build the dependencies in the documented and supported way - which according to https://gitlab.gnome.org/GNOME/libxslt/-/tree/master/win32 is not using CMake - to avoid odd side effects, such as the naming of libraries that we've seen change when building OpenSSL 1.x and zlib.

I can see using CMake here is more consistent, gives us a pkg-config file, and works (and presumably will continue to work in the future as long as the pkg-config file is correct). Will it continue to work in the future though?

davecramer commented 2 months ago

I wasn't aware of that document. That said the document is 22 years old.

dpage commented 2 months ago

Fair point :-D

anarazel commented 2 months ago

I'm on the fence about this change (although other-Dave committed it already). The original intent of this project was to build the dependencies in the documented and supported way - which according to https://gitlab.gnome.org/GNOME/libxslt/-/tree/master/win32 is not using CMake - to avoid odd side effects, such as the naming of libraries that we've seen change when building OpenSSL 1.x and zlib.

Given that libxslt is maintained by the same folks as libxml and that you were already building libxml using cmake and that none of the top-level documents in libxslt reference the build process this was using before, using cmake IMO is the safer bet.

It's probably also worth mentioning that cmake+msvc is tested in CI upstream, whereas the cscript approach is not: https://gitlab.gnome.org/GNOME/libxslt/-/blob/master/.gitlab-ci.yml#L162

anarazel commented 2 months ago

Oh, and thanks for merging!