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
[poppler] Add new port #15158
[poppler] Add new port #15158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @playgithub
Thanks for your contribution! 😄
I pointed out a few things to improve the port. Please let me know if you need any further guidance with this PR.
Hi @playgithub Could you please sign CLA first? |
Done |
Could you please look into the failures on x64-linux? something like this:
Please see the details from |
I have no machine running Linux now. |
Since the failures are from test modules. You can disable https://github.com/freedesktop/poppler/blob/master/CMakeLists.txt#L753 Would you like to try this? @playgithub |
For commit 965ed9d, the github checks blocks for more than 10 hours. |
Anyway to see the list in vcpkg which has installed a lot of libs? |
When building using the triplet below for C++17 x64-windows-cpp17.cmake set(VCPKG_TARGET_ARCHITECTURE x64)
set(VCPKG_CRT_LINKAGE dynamic)
set(VCPKG_LIBRARY_LINKAGE dynamic)
set(VCPKG_CXX_FLAGS "/std:c++17")
set(VCPKG_C_FLAGS "/std:c17") By far I encountered 2 problems:
I'm not sure if using a triplet to build in C++17 is an efficient way, because I have to pause my job until it is solved. |
"name": "devil", | ||
"platform": "(windows | linux) & !arm" | ||
}, | ||
"fontconfig", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like an optional dependency.
https://github.com/freedesktop/poppler/blob/master/CMakeLists.txt#L132
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WITH_FONTCONFIGURATION_FONTCONFIG
is true when while "platform": "!(windows | android)"
"platform": "!(windows | android)" | ||
}, | ||
"freetype", | ||
"libiconv", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also an optional dependency. iconv
is only required in cpp module, which might be considered to add a feature.
https://github.com/freedesktop/poppler/blob/master/CMakeLists.txt#L230
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For C++ ENABLE_CPP
is always true.
"platform": "osx" | ||
}, | ||
{ | ||
"name": "devil", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see any places used devil
in source codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but without devil
it failed to pass checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to figure out where and how it is used in this port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked the include
files, no devil
.
Could you please help review this PR again? Thanks. |
I'm not sure if the dependency lists are suitable. So I add |
Thanks for the PR @playgithub |
[poppler] Add new port (microsoft#15158)
vcpkg install poppler[cpp17]
, it builds in C++17PS: for poppler C++14 and C++17 are not compatible