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.

Sign up for GitHub

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

Jump to bottom

[cairo,cairomm] add fontconfig and quartz feature and update cairo version #16825

Merged
merged 1 commit into from
Apr 19, 2021
Merged

[cairo,cairomm] add fontconfig and quartz feature and update cairo version #16825

merged 1 commit into from
Apr 19, 2021

Conversation

abique
Copy link
Contributor

@abique abique commented Mar 22, 2021

No description provided.

@abique abique changed the title [cairo] fontconfig should not be requiered if x11 is not enabled [cairo] fontconfig should not be required if x11 is not enabled Mar 22, 2021
@JackBoosY JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label Mar 23, 2021
JackBoosY
JackBoosY suggested changes Mar 23, 2021
View reviewed changes
Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please bump the port version. See documentation.
Also please run command vcpkg x-add-version --all then commit the changes.

@ghost
Copy link

ghost commented Mar 23, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@abique
Copy link
Contributor Author

abique commented Mar 23, 2021

@JackBoosY Thank you very much for your feedback. I've updated the pull request accordingly.

@JackBoosY
Copy link
Contributor

Please get failure logs here.

@abique
Copy link
Contributor Author

abique commented Mar 25, 2021

To me it looks line a problem in the other packages.

@abique abique force-pushed the patch-1 branch 2 times, most recently from bfb7d89 to a35226d Compare March 25, 2021 16:32
@abique
Copy link
Contributor Author

abique commented Mar 25, 2021

I've made fontconfig a feature, that way it can be disabled.

@abique
Copy link
Contributor Author

abique commented Mar 25, 2021

Please bump the port version. See documentation.
Also please run command vcpkg x-add-version --all then commit the changes.

Thank you for that, this is now addressed.

@abique abique requested a review from JackBoosY March 25, 2021 16:44
@abique abique changed the title [cairo] fontconfig should not be required if x11 is not enabled [cairo] add fontconfig feature Mar 25, 2021
@abique abique changed the title [cairo] add fontconfig feature [cairo] add fontconfig and quartz feature Mar 26, 2021
@JackBoosY
Copy link
Contributor

Please get failure logs here.
The osgearth regression is not related to this PR.

Waiting for #16904 merged.

@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Mar 26, 2021
@abique
Copy link
Contributor Author

abique commented Mar 29, 2021

I had to rework cairomm because it was trying to compile quartz on Linux and Windows which obviously will fail.

@abique abique changed the title [cairo] add fontconfig and quartz feature [cairo,cairomm] add fontconfig and quartz feature and update cairomm version to match cairo Mar 29, 2021
@JackBoosY JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Mar 29, 2021
@JackBoosY
Copy link
Contributor

Get failure logs here.

@Neumann-A
Copy link
Contributor

this PR collides with #13100. Also could have switched the build to the native buildsystem via vcpkg_configure_make which will be required anyway.

@abique
Copy link
Contributor Author

abique commented Mar 31, 2021 •
edited
Loading

this PR collides with #13100. Also could have switched the build to the native buildsystem via vcpkg_configure_make which will be required anyway.

#13100 is massive, and it does not address the same issues. While this one has a minimum amount of changes and brings cairo into a better shape. You now can have freetype without using fontconfig, and it provide better support for quartz on mac.

I'd merge #16825 first.

@Neumann-A
Copy link
Contributor

Yeah but the question is if the native buildsystem supports the new features introduced here. If not this PR has to remove the features added here.

@abique
Copy link
Contributor Author

abique commented Mar 31, 2021 •
edited
Loading

Of course you can enable/disable fontconfig or quartz when using the configure script.

@abique abique force-pushed the patch-1 branch 2 times, most recently from cb7cf04 to 97c596b Compare March 31, 2021 11:02
@abique
Copy link
Contributor Author

abique commented Mar 31, 2021

I've bumped cairo to 17.4 because 1.16.0 had a bunch of known bugs.

@Hoikas
Copy link
Contributor

Hoikas commented Mar 31, 2021

A previous cairo update PR was shelved because 1.17.x are snapshots. #16036 (comment)

@abique
Copy link
Contributor Author

abique commented Mar 31, 2021

I have a crash in cairo, which I'll confirm soon if it is fixed by 1.17.4.
I believe 1.16.0 to be less stable than 1.17.4 despite the snapshot denomination.

@abique
Copy link
Contributor Author

abique commented Mar 31, 2021

To be precise we're running into https://gitlab.freedesktop.org/cairo/cairo/-/issues/412

@abique abique force-pushed the patch-1 branch 2 times, most recently from 65ff0da to 9efed17 Compare March 31, 2021 11:55
@abique abique changed the title [cairo,cairomm] add fontconfig and quartz feature and update cairomm version to match cairo [cairo,cairomm] add fontconfig and quartz feature and update cairo version Mar 31, 2021
@JackBoosY
Copy link
Contributor

Should we apply this patch?

@abique
Copy link
Contributor Author

abique commented Apr 1, 2021

After some testing we figured that even 1.17.4 did not fix our issues on macos.
We are going to check if switching from static linking to dynamic linking fixes it.
Sorry for the noise, I'll come back with the conclusion.
Through, archlinux is using 1.17.4 and if that version was broken we'd know it. I've checked the changelog and basically the big change is the new backend cogl. Appart from that it is mostly fixes.

@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Apr 2, 2021
@JackBoosY
Copy link
Contributor

After some testing we figured that even 1.17.4 did not fix our issues on macos.
We are going to check if switching from static linking to dynamic linking fixes it.
Sorry for the noise, I'll come back with the conclusion.
Through, archlinux is using 1.17.4 and if that version was broken we'd know it. I've checked the changelog and basically the big change is the new backend cogl. Appart from that it is mostly fixes.

So should we wait for upstream fix this issue completely?

@abique
Copy link
Contributor Author

abique commented Apr 2, 2021

After some testing we figured that even 1.17.4 did not fix our issues on macos.
We are going to check if switching from static linking to dynamic linking fixes it.
Sorry for the noise, I'll come back with the conclusion.
Through, archlinux is using 1.17.4 and if that version was broken we'd know it. I've checked the changelog and basically the big change is the new backend cogl. Appart from that it is mostly fixes.

So should we wait for upstream fix this issue completely?

We don't understand why, but switching from static build to dynamic build seems to fix it.
We'll get back with more info.

@abique
Copy link
Contributor Author

abique commented Apr 12, 2021

We've been using 1.17.4 for some time now on Windows and Macos and it works like a charm. Plus Archlinux is using this version since a lot of time.

I think it is safe, and the right move to jump to this version.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Apr 12, 2021
@abique
Copy link
Contributor Author

abique commented Apr 15, 2021

As you don't feel excited about cairo-1.17.4 I'll revert to 1.16.0 so at least our work is not lost.

@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Apr 15, 2021
@abique
Copy link
Contributor Author

abique commented Apr 16, 2021

@PhoebeHui Hi,
Shall we merge it?

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Apr 18, 2021
@JackBoosY
Copy link
Contributor

Ping @strega-nil for review and merge this PR.

@strega-nil strega-nil merged commit 4c830b1 into microsoft:master Apr 19, 2021
@strega-nil
Copy link
Contributor

Thanks @abique :)

@abique abique deleted the patch-1 branch April 20, 2021 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hoikas Hoikas Hoikas left review comments

@JackBoosY JackBoosY JackBoosY approved these changes

@PhoebeHui PhoebeHui Awaiting requested review from PhoebeHui

Assignees

@JackBoosY JackBoosY

Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@abique @JackBoosY @Neumann-A @Hoikas @strega-nil