-
Notifications
You must be signed in to change notification settings - Fork 54
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
Release 5.1 - Support current compilers, nodejs 16+ support #794
Conversation
rm -fr packages/boost mkdir packages/boost bcp --boost=$HOME/Downloads/boost_1_82_0 boost/container/vector.hpp packages/boost
Circle CI no longer provide Xcode 11. > Job was rejected because resource class medium, image xcode:11.2.1 is > not a valid resource class
In case insensitive file systems (macOS) `version` may be treated as a header in the search path resulting in a build failure.
NodeJS 16 onwards no longer supports building with c++11 See nodejs/node#38367
Circle CI macOS image is incompatible with the older ruby-ffi and related dependencies
The older image appears to have issues with CircleCI in regards to SSH key setup to clone drafter.
As much as it disappoints me to allow to allow warnings to be passed in CI. Currently warnings are emitted when compiling the tests (Catch2 upgrade) and also from legacy sundown package (external-ish dependency). Given the product maturity and lack of maintenance will allow warnings.
@@ -51,4 +59,4 @@ DEPENDENCIES | |||
cucumber | |||
|
|||
BUNDLED WITH | |||
1.10.3 | |||
2.4.6 |
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.
All changes to the gemfile are to work with newer bundle. The Circle CI image no longer works out the box with older Ruby.
Note, Ruby is a functional test dependency only. Has no bearing on consumption of Drafter.
@@ -246,7 +246,7 @@ jobs: | |||
- run: brew install cmake | |||
- <<: *clang-debug | |||
macos: | |||
xcode: "11.2.1" | |||
xcode: "14.2.0" |
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.
CircleCI no longer support Xcode 11, neither do Apple. Not feasible to test anymore.
@@ -145,7 +145,7 @@ jobs: | |||
test-gyp: | |||
<<: *test-base-legacy | |||
docker: | |||
- image: gcc:4.8.5 | |||
- image: gcc:7 |
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.
GCC 4 image appears to no longer allow successful clone of project (CI stalls on git clone itself using GCC 4). Given GYP test is mostly for Protagonist, I am not concerned if GCC 4 is no longer supported, given that no modern supported version of NodeJS support GCC 4.
@@ -4,7 +4,7 @@ aliases: | |||
- &test-base-legacy | |||
working_directory: /tmp/drafter | |||
environment: | |||
CXXFLAGS: '-Werror -Wno-error=unused-function' | |||
CXXFLAGS: '-Wno-error=unused-function' |
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's a little disappointing to remove Werror (although its only in legacy
branch of CI). There are warnings when built with current/modern compiler versions.
@@ -105,7 +105,7 @@ | |||
[ 'OS in "linux freebsd openbsd solaris android"', { | |||
'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', '-Wno-comment' ], | |||
'cflags_cc!': [ '-fno-rtti', '-fno-exceptions' ], | |||
'cflags_cc': [ '-std=c++11' ], | |||
'cflags_cc': [ '-std=c++14' ], |
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.
NodeJS requires C++14 onwards, the gyp (a build system used by Node) is used by Protagonist, the Drafter NodeJS bindings. Change needed for NodeJS 16 compatibility.
@@ -8,7 +8,7 @@ SEPARATOR="---------separator---------" | |||
echo "libapib" > $DESTINATION | |||
echo $SEPARATOR >> $DESTINATION | |||
( | |||
echo "Boost $(cat packages/boost/VERSION)" >> $DESTINATION | |||
echo "Boost $(cat packages/boost/VERSION.txt)" >> $DESTINATION |
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 particular file is renamed from VERSION
to VERSION.txt
as on case insensitive filesystems VERSION
matches version
which compilers try to lookup during #import <version>
. The old file name causes compilation errors. (Its a file I made in the past when I made this script back in the day in 646e15e)
What:
This change releases Drafter 5.1 (Drafter, the API Blueprint Parser written in C/C++). This release contains existing bug fixes that had been merged a few years back and never got released. Along with numerous changes for CI to currently pass, and for the project to build in modern systems, and Node JS 16.
Both Catch (test library dev dependency) and Boost are upgraded. Boost is vendored in, this is done in its own commit to make review easier as it is a "large" change to boost source. Catch2 older version no longer allows project to be built on current compilers and faced similar issue.
Why:
Overall, fighting bit rot.
Criticality:
Medium
Blast radius:
Low - No change to API or ABI Compatibility. Unexpected to change any minimum compiler requirements of the project.
Expected outcome:
Not a lot changes.
Potential impact and visibility:
No expected impact.
Test results:
All existing test coverage passes (unit, integration).
I am unable to test the project still builds on windows. Appveyor (our windows CI) doesn't seem to be anything that's still being paid for. It's unlikely that Windows incompatibilities have arisen given only functional change is to boost (which they should have sufficient tests for).