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

Use future boost::gil 1.68 IO extensions instead of homegrown hack #2221

Merged
merged 2 commits into from Aug 26, 2018

Conversation

Projects
None yet
5 participants
@adrianbroher
Copy link
Contributor

commented Jul 17, 2018

@jbleich reported in #2211 compilation errors when building FO against the upcoming boost 1.68 release, where one chunk of the reported issues were caused by our homegrown hacks to enable PNG gray-alpha support within boost.gil because of an API change.

Instead of implementing support for both pre- and post boost 1.68 gil I decided to phase out our gilext implementation in favor of the boost.gil gray-alpha image and PNG loading support. When linking pre-1.68 boost implementations the build system still uses gilext. Starting with 1.68 boost.gil the corresponding IO extension is used and gilext is ignored.

While testing the new implementation I found a bug in boost.gil and reported it to upstream (boostorg/gil#117). When we're lucky the bug is fixed in time for the boost 1.68 release. In case upstream doesn't fix the bug in time we may need to fix this issue in another way (probably converting the offending textures in a more consumable PNG variant).

@adrianbroher adrianbroher force-pushed the use-upstream-gil branch from 4396072 to 2029614 Jul 17, 2018

@adrianbroher adrianbroher changed the title Use future boost::gil 1.68 IO extensions instead of homegrown hack WIP: Use future boost::gil 1.68 IO extensions instead of homegrown hack Jul 17, 2018

@adrianbroher adrianbroher changed the title WIP: Use future boost::gil 1.68 IO extensions instead of homegrown hack Use future boost::gil 1.68 IO extensions instead of homegrown hack Jul 17, 2018

@adrianbroher adrianbroher self-assigned this Jul 17, 2018

@Vezzra Vezzra added this to the post 0.4.8 milestone Jul 18, 2018

@adrianbroher adrianbroher force-pushed the use-upstream-gil branch from 2029614 to b489721 Jul 18, 2018

@adrianbroher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2018

@Vezzra I disagree with the post 0.4.8 MS assignment. boost 1.68 is close to release and people complaining about 1.68 incompatibility will come for sure. I'm waiting for upstream to respond so this PR can merged and cherry picked to 0.4.8.

@Vezzra

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

@adrianbroher, ok, no objection from my side. You didn't assign a milestone yourself, and I thought the workarounds currently in place were sufficient, but if you want to include this into the release, that's fine with me.

However, in this case we'll have to postpone RC3 (unless you manage to get the PR merged and cherry picked until tomorrow - Thursday - noon of course). How much time do you think you'll need?

@adrianbroher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2018

How much time do you think you'll need?

That's not up to me, but the boost.gil maintainers. Depending on when they merge boostorg/gil#118 and the boost release manager agrees to cherry picking into the boost 1.68 beta this can be merged or I need to think of a contingency plan.

@Vezzra

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

I see that boostorg/gil#118 has been merged, hopefully the decision of the boost release manager if they are going to cherry pick that into the boost 1.68 beta will be made soon?

If it's not going to be cherry picked, how long will it take you to implement a contingency plan?

RC3 will be postponed, that much is already decided, the question is, until when? I can offer these dates for RC3: July 22nd, July 27th, July 29th. I don't want to delay RC3 beyond July 29th though, unless it's really necessary.

@jbeich

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

On FreeBSD this PR also requires boostorg/gil#119 (build log: before vs. after) but I plan to include the fix in downstream package, anyway.

@Dilvish-fo

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

@adrianbroher I see that one of the boost devs just added your PR there to the 1.69 milestone, and noted a bit of concern about it having been merged into the dev branch prior to helping you complete the testing aspect. He did make an Issue and post a bit of code to try facilitating that, and if that testing works out promptly then I suppose you could ask him to consider redesignating the milestone as 1.68.

But it looks pretty likely that we'll need you to follow up on

or I need to think of a contingency plan.

@adrianbroher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2018

@Dilvish-fo thanks for pointing out, but I'm already working on fixing this upstream.

@jbeich

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

boostorg/gil#118 and boostorg/gil#119 are part of Boost 1.68.0 since RC1

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

Master works fine for me with this applied. @jbleich does this fix compilation with Boost 1.68 for you as well?

@jbeich

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

#2211 was reported by me, not @jbleich. I confirm, FreeOrion builds fine after applying this PR, see build log.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

Sorry; typo.

In that case, any objections to merging this and cherry picking to v0.4.8 ?

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

Before I focused on the forum issues I tried to build and test this PR with master and also with the release branch. Which worked fine on OSX, but on Windows I got build errors. I intended to try again on Sunday, but will probably again be too busy with the forum.

However, apparently you can build and run master with this PR just fine, so it's most likely an issue with my dev environment. Can you try to build and run this PR with the release branch too? Because the release branch needs to be built with a different SDK version (v8, not v10), and I want to be sure that the PR works ok with both SDKs on Windows, before we merge and cherry pick it.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

The release and SDK8 are seemingly still using MSVC2015, so I had to switch the relevant MSVC project file modification from the 2017 to the 2015 project files.

Otherwise OK to merge this to master then?

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 4, 2018

The release and SDK8 are seemingly still using MSVC2015, so I had to switch the relevant MSVC project file modification from the 2017 to the 2015 project files.

Have you been able to successfully build FO with MSVC2015? Because I too noticed that @adrianbroher didn't update the MSVC2015 project (after my first failed attempt to build the PR on Windows), so I did it myself. I did it by editing the relevant .vcxproj file with a text editor though, so maybe I messed something up, because I still couldn't build the PR, neither with master nor the release branch.

I haven't switched to MSVC2017 yet - are we already deprecating support for 2015 in master?

Which MSVC version does our AppVeyor CI configuration use?

Otherwise OK to merge this to master then?

As I said, if you can build and run the PR on Windows, sure... however, if you have only tested with MSVC2017 and still want to maintain 2015, maybe you should try to build with 2015 also.

Otherwise, no objection to merge this. Cherry picking however should only be done if the PR also builds and runs ok with the release branch (on Windows, that is, on OSX everything works fine anyway).

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 4, 2018

I applied the project file changes here to the MSVC 2015 project files in a release branch variant of this, which worked locally with SDK 8, but the AppVeyor build ignored the changes apparently so had related errors. I've since modified the changes in the release branch PR to not require project file or CMake modifications. I can build that build locally and it works in the release branch test builds... at least for Windows and OSX; the Travis test builds hadn't even started after 5 hours or so, so I pushed a further commit in part to restart them. I'd probably port those release branch changes back to this pull request before or after before merging.

AppVeyor uses the MVSC 2015 compiler (version 14.0) as far as I can tell.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

Switched this to use direct checks of BOOST_VERSION as is currently done for the release branch pull request. Should thus work for all build systems without other modifications.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

@jbeich does this still work for you with Boost 1.68 ?

@adrianbroher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2018

Nice breakage. How about learning to properly use VS?

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

Nice breakage. How about learning to properly use VS?

Are you referring to my editing the MSVC2015 project file with a text editor, or @geoffthemedio's fix replacing the GIGI_CONFIG_USE_OLD_IMPLEMENTATION_OF_GIL_PNG_IO preprocessor define with direct checks of the boost version?

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

Can build and run this PR successfully now on Windows, OSX and Linux.

@adrianbroher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@Vezzra

Are you referring to my editing the MSVC2015 project file with a text editor, or @geoffthemedio's fix replacing the GIGI_CONFIG_USE_OLD_IMPLEMENTATION_OF_GIL_PNG_IO preprocessor define with direct checks of the boost version?

The latter. Deleting the target_compile_definitions statement via 5e0d924 not only removes the GIGI_CONFIG_USE_OLD_IMPLEMENTATION_OF_GIL_PNG_IO preprocessor define, it disables grayalpha support within boost.gil 1.68 and later. Also what's the point of deleting this whole check? It works fine as it is as long as the define is added to all build configurations in MSVC (Release, Release-XP and Debug).

Which MSVC version does our AppVeyor CI configuration use?

  • On master: MSVC 2017 with vs140_xp toolkit (the one distributed with MSVC 2015)
  • On release-v0.4.8: MSVC 2015 with vs140_xp toolkit (the one distributed with MSVC 2015)

but the AppVeyor build ignored the changes apparently so had related errors.

AppVeyor uses the Release-XP build configuration, you probably only modified the Release build configuration, that's why the build failed.

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Finally got around to test @adrianbroher's original implementation with the GIGI_CONFIG_USE_OLD_IMPLEMENTATION_OF_GIL_PNG_IO preprocessor define added to all build configurations in the MSVC2015 project.

As @adrianbroher said, that solved the build errors I got on Windows. I tested both with master and the release branch, in both cases building and running worked perfectly fine.

Which means we can reset this PR to commit e51bfb5 and the PR for the release branch (#2241 ) to 966b7d1. Then add the GIGI_CONFIG_USE_OLD_IMPLEMENTATION_OF_GIL_PNG_IO preprocessor define to all build configurations of the MSVC projects (2015&2017 on master, only 2015 on the release branch), and these PRs should be ready for merging.

Did I miss anything?

I can apply these changes to the PR for the release branch, if you want me to, but the changes for this PR (master) I have to leave to somebody else, as I don't have MSVC2017 installed on my Windows system yet.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

@Vezzra I suggest applying your adapted version to the release, applying the same commit to master, then having an additional commit in master that just does the MSVC2017 project file adjustment.

@adrianbroher I didn't notice the part where BOOST_GIL_IO_ENABLE_GRAY_ALPHA was defined if the boost version was >= 1.68 in the CMakeLists.txt changes. It seemed simpler to test the Boost version at the two places in code where relevant in order to avoid needing to set up all the MSVC project files with an additional preprocessor define (GIGI_CONFIG_USE_OLD_IMPLEMENTATION_OF_GIL_PNG_IO), particularly since that won't be able to automatically adjust to the Boost version being used. Does BOOST_GIL_IO_ENABLE_GRAY_ALPHA needs to be defined anywhere other than in the two .cpp files that are being modified, or could it just be defined just before the include <boost/gil/extension/io/png.hpp> line? This still seems easier than needing to manually adjust the defines in multiple projects and solutions for MSVC when transitioning between pre and post Boost 1.68.

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

I suggest applying your adapted version to the release

Ok, I reverted the PR for the release branch to 966b7d1 and applied my changes.

Cherry picking that commit to master doesn't work though, I get conflicts because the MSVC project files apparently have diverged in the master and release branches, and those project files are treated as binary files so merging does not work.

The changes need to be repeated for master manually (which isn't a big deal, just adding the GIGI_CONFIG_USE_OLD_IMPLEMENTATION_OF_GIL_PNG_IO define in the GiGi project to all build configurations).

However, before I go ahead and revert this PR also and apply these changes, we should decide if we want it do that way or go with your suggestion:

Does BOOST_GIL_IO_ENABLE_GRAY_ALPHA needs to be defined anywhere other than in the two .cpp files that are being modified, or could it just be defined just before the include <boost/gil/extension/io/png.hpp> line? This still seems easier than needing to manually adjust the defines in multiple projects and solutions for MSVC when transitioning between pre and post Boost 1.68.

In that case reverting to e51bfb5 probably shouldn't be done?

@Vezzra Vezzra modified the milestones: v0.4.8 (mandatory), post 0.4.8 Aug 13, 2018

@Vezzra Vezzra removed this from Proposed in 0.4.8 Release Aug 13, 2018

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

#2241 has been merged, reassinging this to post release milestone.

Use future boost::gil 1.68 IO extensions instead of homegrown hack
Starting with boost 1.68 boost::gil integrates IO support for
grayscale-alpha png images, so prefer their implementation instead of
our hacky gilext code.

@geoffthemedio geoffthemedio force-pushed the use-upstream-gil branch 2 times, most recently from 347f8f7 to 339b37d Aug 23, 2018

@geoffthemedio geoffthemedio force-pushed the use-upstream-gil branch from 339b37d to ba71437 Aug 23, 2018

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Combined the MSVC 2015 project file changes from the release commits into one commit here.

@geoffthemedio geoffthemedio merged commit ddb1e35 into master Aug 26, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@geoffthemedio geoffthemedio deleted the use-upstream-gil branch Aug 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.