Skip to content
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

libmupdf version bump #2730

Merged
merged 96 commits into from
Feb 17, 2018
Merged

libmupdf version bump #2730

merged 96 commits into from
Feb 17, 2018

Conversation

glachancecmaisonneuve
Copy link
Contributor

Note: the new third_party/lcms include is not used to link to lcms.

@ras0219-msft
If you have a minute to spare, could you tell me why it's preferable to maintain a separate build system that uses cmakelists instead of upgrading the provided vcproj (with devenv /upgrade) and using msbuild? (ref: pull/1950 - my original portfile which was rejected for being msbuild based). You have some insight on this topic I am lacking because to me, it's a lot more work. For example for this version change, I had to create a patch to fix some includes in some c files, which took a while.

Meanwhile, running devenv /upgrade on platform/win32/libmupdf.vcproj and tweaking the AdditionalIncludeDirectories took very little work. (somehow msbuild makes up for missing includes).
Using the library creator's provided build system rather than writing our own seems to me less risky.

I have always thought that a difference of opinion in how something should be done is always the result of one of the 2 parties having some information that the other does not. If you have a minute to spare to educate me, I am all ears.

miurahr and others added 30 commits February 5, 2018 12:21
Libgta is a portable library that implements the Generic Tagged Array (GTA)
file format.

Signed-off-by: Hiroshi Miura <miurahr@linux.com>
Signed-off-by: Hiroshi Miura <miurahr@linux.com>
- bzip2, zlib and liblzma

Signed-off-by: Hiroshi Miura <miurahr@linux.com>
Signed-off-by: Hiroshi Miura <miurahr@linux.com>
add lf at end of file.
Allows PowerShell to change the name of the downloaded directory correctly.
Adding Torch's TH library
* [nghttp2]: update to 1.30.0

* [nghttp2] Enable static builds
Signed-off-by: Hiroshi Miura <miurahr@linux.com>
sobjectizer updated to v.5.5.21
Instead, use -Command with the appropriate number of escaped quotes
(which ended up being 3)
brucedjones and others added 25 commits February 13, 2018 16:13
* Fix for issue #2786

* [vcpkg-cmake-toolchain] Only applocal dependencies for shared libraries
* [vcpkg] Add Default-Feature to make_status_pgh utility function

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Parse "Default-Features" as dependencies and add test for parsing

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Document some methods and structures

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Add install_default_features_test

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Change install_default_features_test to not have preinstalled package

* [vcpkg] Test install behaviour of default features

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Implement default features

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Test default features upgrade behavior

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Implement upgrade with default features

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Test behaviour of upgrade with default features in dependencies

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Make upgrade install new default features

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Move collecting of packages for which to prevent defaults

Further down the line to create_feature_install_plan.

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Fix core missing from default features and potential inf loop

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Rename, fix and move some tests

Signed-off-by: Squareys <squareys@googlemail.com>
I was a bit confused to see a reference to "1.65.1" while I was installing 1.66.0, but it turns out this URL is just not updated. This updates it.
* update cuda requirement to 9.0, fixes #2791

* [cuda] Restore sample version blob
Pacman of Msys understands only lowercase environment variables
http_proxy and https_proxy.
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
* [portaudio] Added ASIO support to build

* Update libpng to 1.6.34

* [liblo] Initial port

* Revert "Update libpng to 1.6.34"

This reverts commit ede0bb9.

* Revert "[liblo] Initial port"

This reverts commit bb819eb.

* [liblo] Initial port

* [liblo] Use vcpkg_from_github() and vcpkg_fixup_cmake_targets()

* [liblo] Fix SHA512
When a package dependency was not found (has no source control file),
install would exit with "Value was null" when trying to install its default
features, as the dependency would be marked erroneously as found in this
case.

Signed-off-by: Squareys <squareys@googlemail.com>
* add qt5-quickcontrols2

* add qt5-quickcontrols port

* add qt5-graphicaleffects
[lmdb] Fix possible whitespace problem
[libevent] Fix generated libevent targets files
…upport feature packages (#2687)

[corrade,magnum,-plugins,-extras,-integration] Update to latest and support feature packages
* [vcpkg] Add find/find_installed/is_installed for FeatureSpec

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Fix build command for packages that depend of features

Signed-off-by: Squareys <squareys@googlemail.com>
@ras0219-msft ras0219-msft merged commit 995ab09 into microsoft:master Feb 17, 2018
@ras0219-msft
Copy link
Contributor

Thanks for the PR! It looks like GitHub exploded a bit when I merged to master, so sorry for any trouble that caused on your side :(

If you have a minute to spare, could you tell me why it's preferable to maintain a separate build system that uses cmakelists instead of upgrading the provided vcproj (with devenv /upgrade) and using msbuild?

First, thank you for asking so professionally -- I really appreciate it! I'll also apologize in advance for the wall of text.

I have prepared a branch for what it would look like to use MSBuild [1]. In terms of raw, local maintainability, the CMake approach takes ~30 lines in the portfile and ~45 lines in the rewritten buildsystem. In the MSBuild approach, the portfile takes ~100 lines (assuming no patches). This isn't a perfect measurement by any means, but it gives us an initial benchmark for the complexity, which seems to slightly favor the rewrite. Additionally, while subjective, I find the actual code in the MSBuild variant to be harder to understand, with more details that could go wrong.

Next is the immediately available flexibility in build configurations. The CMake variant can build DLLs, static libs that dynamic link the CRT, AND static libs that static link the CRT. The MSBuild variant can only build static libs that static link the CRT. This means that it can't work out-of-the-box for a new user (vcpkg install libmupdf) because the default in VS is to dynamic link the CRT and your program will fail to link (or worse) if you try to mix those settings. The CMake variant also gives us quite a fighting chance (with no changes!) at supporting ARM/ARM64 in the future as long as there isn't anything explicitly tied to x86 in the code. The MSBuild variant will at the minimum require patching in support for the platform. Same goes for UWP and any other platforms we might want to support in the future.

Then, there is the case of the internal copies of third party dependencies. As you already know, one of the reasons the MSBuild solution is so easy to get working initially is because there are copies of all the dependencies' source code in the repo. This means that the built libmupdf will at best be overbuilding dependencies if you choose to also use freetype elsewhere in your program. The average case is that you get failures at link time (ODR violation) due to multiple symbol definitions because libmupdf will bring its own copy of FT_GlyphSlot_Own_Bitmap, a function owned by freetype. The worst case is that you get random runtime failures in production because the linker resolved the previous conflict in an unfortunate way.

The worst case above is made infinitely more likely in the case of LCMS2, which libmupdf has hard-forked. From the documentation[2]:

  1. Every API function (or at least those that might allocate) now takes
    a cmsContext pointer.

Because these are C libraries this means that while the symbol name is the same, the ABI is completely incompatible and is guaranteed to cause crashes at runtime that are a nightmare to debug. This means there is no safe way to consume the upstream lcms2 AND libmupdf in the same application if you use the vendored copy (which the existing MSBuild does).

Due to these ABI issues, we would certainly need to opt it out of our MSBuild automatic linking. Any project that wants to use it would be barred from using a huge number of other libraries in our catalog (because of ODR violations).

There's also the issue that the copies of dependencies inside it can't be separately updated, which almost certainly means unpatched security vulnerabilities (a huge deal in a pdf processing library!).

It's totally possible to write an application that doesn't hit any of the above problems -- if all you need is static linking the CRT, and you don't ever load untrusted PDFs, and you don't want to use Qt/POCO/OpenCV/harfbuzz/freetype/glfw3/vtk/itk/curl/openssl/etc, then the MSBuild solution works great! In Vcpkg, we want to build an ecosystem of libraries that work well together. You shouldn't have to choose between libmupdf and Qt -- if you want to make a great application with them together, we want to help!

[1] https://github.com/Microsoft/vcpkg/tree/dev/roschuma/libmupdf-msbuild
[2] https://github.com/ArtifexSoftware/thirdparty-lcms2/blob/1079c80803b51d0ea50b80ebb9920aa1da467cc0/doc/WhyThisFork.txt

@glachancecmaisonneuve
Copy link
Contributor Author

Thank you so much! That is incredibly generous on your part. I now have a shopping list of new subject matters to learn about because I'm a wholly ignorant of most of what you brought up. From the little I do know however, you have convinced me that the effort is well worth it, especially in situations where third party libraries are involved.

Not that I haven't taken what you said to heart, but it did get me thinking on the new csproj file format that is supposed to be a lot simpler. I will need to take a look at that as well.

@glachancecmaisonneuve glachancecmaisonneuve deleted the libmupdf_12_0 branch February 19, 2018 02:56
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.