Skip to content

[glib, fltk, cairo, python3] Sequence of adjustments for windows and macos#9593

Closed
magicfoo wants to merge 6 commits intomicrosoft:masterfrom
magicfoo:comp
Closed

[glib, fltk, cairo, python3] Sequence of adjustments for windows and macos#9593
magicfoo wants to merge 6 commits intomicrosoft:masterfrom
magicfoo:comp

Conversation

@magicfoo
Copy link

@magicfoo magicfoo commented Jan 8, 2020

This PR provides few minor or more majors adjustments to the following components:

  • fix glib to support static build on windows
  • fix cairo to properly build on macos and support quartz rendering
  • update fltk to support cairo extension
  • fix python3 to support static build on windows

@magicfoo magicfoo changed the title Sequence of commits adjusting fltk + cairo + python3 on windows and macos [fltk, cairo, python3] Sequence of adjustments for windows and macos Jan 8, 2020
@magicfoo magicfoo changed the title [fltk, cairo, python3] Sequence of adjustments for windows and macos [glib, fltk, cairo, python3] Sequence of adjustments for windows and macos Jan 8, 2020
@magicfoo magicfoo force-pushed the comp branch 2 times, most recently from 8706849 to a36a356 Compare January 8, 2020 22:43
+ Fix compilation issues with Xcode 11.3 (catalina)
@magicfoo
Copy link
Author

magicfoo commented Jan 9, 2020

Interesting to see that the original order of the commits in this stack is displayed differently here. I don't guarantee a random order will build properly. Preferably, glib < cairo < fltk < fluid < python < yaml.
Note: Patch on fluid could be a better PR on the fltk repo itself than a patch here ...

@PhoebeHui
Copy link
Contributor

/azp run

Comment on lines 24 to 31
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please merge the code in two 'if (VCPKG_LIBRARY_LINKAGE STREQUAL static)' ?

if (VCPKG_LIBRARY_LINKAGE STREQUAL static)
    vcpkg_apply_patches(
        SOURCE_PATH ${SOURCE_PATH}
        PATCHES
          0001-Static-library.patch
          0003-Fix-header-for-static-linkage.patch
          0004-Remove-linktime-global-optimization.patch
    )
endif()

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, function is deprecated, please use a symbol which resolves to an empty string when not necessary

Copy link
Author

Choose a reason for hiding this comment

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

0002 collides with 0001 and 0004, so it can't be re-ordered. That said, it is not easy to dissociate the CRT and LIB mode patches. Also, keeping this granularity could be great. But we don't want 4^2 patch files for all combinations of CRT (static or not) and LIB (static or not). Any suggestion? Easy way could be to consider a unique static or not cfg build vs dynamic.

@PhoebeHui
Copy link
Contributor

@magicfoo, thanks for the PR!

The following port failed on CI, could you take a look? for glib:x64-windows-static, it passed on CI testing, could you remove it from from ci.baseline, if some of the failures are expected, please also add it to ci.baseline file.

Failed on x86-windows/x64-windows:
REGRESSION: gmime:x86-windows
REGRESSION: harfbuzz:x86-windows
REGRESSION: glibmm:x86-windows

Failed on x64-windows-static:
REGRESSION: glibmm:x64-windows-static
REGRESSION: lcm:x64-windows-static
REGRESSION: fluidsynth:x64-windows-static
REGRESSION: pango:x64-windows-static
REGRESSION: openscap:x64-windows-static
REGRESSION: atk:x64-windows-static
REGRESSION: gdk-pixbuf:x64-windows-static
PASSING, REMOVE FROM FAIL LIST: glib:x64-windows-static (C:\vsts_work\3\s\scripts\ci.baseline.txt)
REGRESSION: gts:x64-windows-static

Copy link
Contributor

Choose a reason for hiding this comment

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

modifications from here going on are whitespace-only, am I right?
if so, can you please remove them in order to simplify patchset?

Copy link
Contributor

Choose a reason for hiding this comment

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

vcpkg_apply_patches is deprecated. Can you please define something like

if (VCPKG_TARGET_IS_WINDOWS)
    set(WIN_PATCH "fix-POSIX_name.patch")
endif()

and then use ${WIN_PATCH} directly above? in this way it is applied only on windows without using a deprecated function (which might trigger a fatal error if the user does not delete buildtrees folder every time!)

Copy link
Author

Choose a reason for hiding this comment

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

This is more tricky AFAIK. The CRT linkage is not part of the triplet and the original source code is shared by vcpkg between the dynamic and static versions of those 2 builds. To prevent this, this port file duplicates the source code locally into 2 working folders named according to VCPKG_CRT_LINKAGE. Then different patches could be applied to those distinct source dirs, especially the patch 0002 building the lib with static CRT.

The very first lines exclude the configuration (lib=dynamic,crt=static) for good reasons. But the config (lib=static,crt=dynamic) is acceptable, but not managed by vcpkg at this level of granularity. I hope I was clear in my explanations here. A consequence of this is the patches can't be applied to the original source code ('from_github') and must be processed later ... I personally agree this is ugly but I don't see atm better option. The best thing to do IMHO is to manage the CRT linkage as the same level the LIB linkage is exposed by VCPKG to the user, extending the triplet x64-windows-static to a quadruplet x64-windows-static-static?? Not ideal, but at least, this is robust.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I lost you. Will need to inspect the port better to understand the problem you are trying to explain

Copy link
Author

Choose a reason for hiding this comment

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

Let me try a shorter version: The original source code can't be patched directly (using PATCHES arg in the vcpkg_from_github function call) as the patches to apply depend on the CRT and LIB linkage modes. This port duplicates the source code to distinctly apply the proper patches on dedicated copies.

Options could be:

  • have vcpkg to generate multiple variants of the source code according to triplet + CRT linkage that could be patched one, and reused later in other builds
  • reverse the patches into a single instance of the source code before to patch again in order to patch dynamically based on the current triplet+CRT

Ideally, we would like vcpkg to do the minimal of work each time we run/resume a build, without to redo everything, be reliable and efficient like some build systems do (tuple, scons, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

we would like vcpkg to do the minimal of work each time we run/resume a build, without to redo everything

that's not how vcpkg works. It deletes everything and restarts from scratch, apart from keeping the uncompressed sources files + patches eventually applied

The triplet is unique, in that a single triplet defines CRT linkage and LIB linkage. So ideally you can apply the idea of having symbols that resolves to nothing depending on the proper configuration at build time

Copy link
Contributor

Choose a reason for hiding this comment

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

also, remember that vcpkg_apply_patches is really broken if used directly in portfiles. It's NOT robust, not at all! It fails always the second time you run an install with the same parameters without manually deleting buildtrees folder

Copy link
Author

@magicfoo magicfoo Jan 10, 2020

Choose a reason for hiding this comment

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

apart from keeping the uncompressed sources files + patches eventually applied

This is exactly the reason why we can't apply those patches on the source files provided by vcpkg but have to duplicate the code per LIB+CRT config.

Copy link
Author

Choose a reason for hiding this comment

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

I rewrote a proposal for the python3 port file which look clean to me. Few things I don't like like the vcpkg_build_msbuild that build for debug and release by itself. At least we should have an option to decide about this behavior. I got around by wrapping the function call forcing one build type at the time.

Copy link
Contributor

@cenit cenit Jan 10, 2020

Choose a reason for hiding this comment

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

This is exactly the reason why we can't apply those patches on the source files provided by vcpkg but have to duplicate the code per LIB+CRT config.

vcpkg calculate a hash for the source files + applied patches. So if the patchset is ok, it recycles the buildtrees source folder, otherwise if the patchset for the new build (different crt linkage, different lib linkage, different os, whatever that makes any symbol be empty for example) is different, the hash will not match and another source tree will be expanded and the appropriate patch files applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you use modern vcpkg routines, you will find that what you are trying to do manually with code duplication and patch manually applied would be handled automatically

@magicfoo
Copy link
Author

magicfoo commented Jan 9, 2020

@magicfoo, thanks for the PR!

The following port failed on CI, could you take a look? for glib:x64-windows-static, it passed on CI testing, could you remove it from from ci.baseline, if some of the failures are expected, please also add it to ci.baseline file.

Failed on x86-windows/x64-windows:
REGRESSION: gmime:x86-windows
REGRESSION: harfbuzz:x86-windows
REGRESSION: glibmm:x86-windows

Failed on x64-windows-static:
REGRESSION: glibmm:x64-windows-static
REGRESSION: lcm:x64-windows-static
REGRESSION: fluidsynth:x64-windows-static
REGRESSION: pango:x64-windows-static
REGRESSION: openscap:x64-windows-static
REGRESSION: atk:x64-windows-static
REGRESSION: gdk-pixbuf:x64-windows-static
PASSING, REMOVE FROM FAIL LIST: glib:x64-windows-static (C:\vsts_work\3\s\scripts\ci.baseline.txt)
REGRESSION: gts:x64-windows-static

The failing build in x64-windows-static are not really regression, as they never work pass before this commit fixing glib in static build. I checked few of them and they fail on a missing intl static lib. They could be fixed later but IMHO should not block this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you delete these three lines instead of commenting them out?

Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you delete these three lines instead of commenting them out?

Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Copy link
Contributor

Choose a reason for hiding this comment

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

This directory should be deleted by vcpkg_fixup_cmake_targets, what files does it contain?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this line? We don't need to include it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use conditions to determine whether changes in these patches will be executed, so that these patches can always be applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Copy link
Contributor

Choose a reason for hiding this comment

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

+ Remove Py_DEBUG in debug build config altering PyModule_Create() symbol
+ Remove conflicting link time (LT) optimizations in Windows profile
@LilyWangL
Copy link
Contributor

Pinging @magicfoo for response. Is work still being done for this PR?

@JackBoosY
Copy link
Contributor

Closing this PR since it seems that no progress is being made. Please reopen if work is still being done.

@JackBoosY JackBoosY closed this May 14, 2020
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.

5 participants