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

Fix build of library on recent Cygwin editions #145

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
2 participants
@jeking3
Contributor

jeking3 commented May 20, 2018

Without this fix you get the behavior in #144.

@raffienficiaud

This comment has been minimized.

Member

raffienficiaud commented Jun 19, 2018

On the branch topic/PR-145-cygwin-fixes, tests cycling.

@@ -12,6 +14,7 @@ project boost/test
: source-location ../src
: requirements <link>shared:<define>BOOST_TEST_DYN_LINK=1
<toolset>borland:<cxxflags>-w-8080
<target-os>cygwin:<define>_POSIX_C_SOURCE=201112L

This comment has been minimized.

@jeking3

jeking3 Jun 20, 2018

Contributor

This is the same fix I put into the appveyor.yml of all affected repositories that use the boost-ci framework.

https://github.com/jeking3/boost-ci/blob/master/templates/appveyor.yml

The only problem here is that the value should be 200112L and not 201112L

This comment has been minimized.

@raffienficiaud

raffienficiaud Jun 20, 2018

Member

So what should I do for the bjam? 200112L or 20112L ?

@jeking3

This comment has been minimized.

Contributor

jeking3 commented Jun 20, 2018

Also do you want a full CI environment (same that now exists on pool, ptr_container, format, uuid, ...?)

@jeking3 jeking3 force-pushed the jeking3:cygwin-build-fix branch from e6d7f14 to 0e4af0e Jun 20, 2018

@raffienficiaud

This comment has been minimized.

Member

raffienficiaud commented Jun 20, 2018

Why not, I have my internal CI on OSX/Linux/Win, and on Win only different versions of Visual. Cygwin might be good to have as well.

@jeking3

This comment has been minimized.

Contributor

jeking3 commented Jun 20, 2018

The point of putting CI here is to allow people to self-serve their own quality of pull requests and to reduce the amount of work it takes for your to qualify a change.

I pulled this PR and a CI PR into my fork:

jeking3#1

Appveyor: https://ci.appveyor.com/project/jeking3/test/build/1.0.6-develop
Travis CI: https://travis-ci.org/jeking3/test/builds/394392360

I haven't looked into the failures yet. Regardless, this particular fix (in this PR) is what I have used to resolve cygwin build issues in the repositories I maintain and in the CMT repositories. In those, the definition is performed in the appveyor script so that Boost.Test builds.

@raffienficiaud

This comment has been minimized.

Member

raffienficiaud commented Jun 20, 2018

You are referring to a CI that is well integrated to GitHub. In all cases, I am running my own tests on the feature branches before merging, the configuration is stable for some years. Said that, you are right that it is always extra work to add a new platform, and I thank you for the ones that you are currently deploying. Let me know once it is stable on your end.

@raffienficiaud

This comment has been minimized.

Member

raffienficiaud commented Jun 20, 2018

Going back to the purpose of the PR: the diff of this is <target-os>cygwin:<define>_POSIX_C_SOURCE=201112L, while the configuration you have on your Appveyor is <target-os>cygwin:<define>_POSIX_C_SOURCE=2001112L. At the end, I am confused on which one to take, my CI is blind to this change, yours is not green

@raffienficiaud

This comment has been minimized.

Member

raffienficiaud commented Jun 21, 2018

Hi again,

I would like to merge this branch asap. Would you please confirm which of the options is correct? 201112L or 2001112L ?

@jeking3

This comment has been minimized.

Contributor

jeking3 commented Jun 22, 2018

@raffienficiaud

This comment has been minimized.

Member

raffienficiaud commented Jun 22, 2018

I just tried on a fresh install of cygwin, and with or without the patch, the code compiles without problem. Just wondering how to reproduce. Any hint?

@raffienficiaud

This comment has been minimized.

Member

raffienficiaud commented Aug 1, 2018

Closing, merged to master/1.68.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment