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

Consume RapidJson through submodule #1828

Merged

Conversation

Projects
None yet
4 participants
@jackgerrits
Copy link
Member

commented Apr 5, 2019

  • Rapidjson is now consumed as a submodule at v1.1.0
  • When using CMake the submodule is automatically checked out if it isn't already there
  • When using VisualStudio the submodule must be explicitly checked out

Fixes #1689

@jackgerrits

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Running a CI image update now, once that's done I will be able to fix the travis build

@jackgerrits

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

It seems the apt-get package isn't available on 14.04. Perhaps this should be done through a submodule instead then

@jackgerrits

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Guidance on the project website is to just copy the rapidjson/include directory. So perhaps as a good first effort we can simply remove all of the extra files that don't need to be there?

@kumpera

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

Wasn't the idea to consume it through a submodule?

@jackgerrits

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

You're right, was hoping we could do it a little easier with packages but it doesn't seem like the way to go

jackgerrits added some commits Apr 5, 2019

@jackgerrits jackgerrits changed the title Consume RapidJson through apt-get and nuget Consume RapidJson through submodule Apr 5, 2019

jackgerrits added some commits Apr 5, 2019

@lokitoth

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

alt text

^ The best kind of code change

Are we also upgrading the version here, or is it a like-for-like substitution?

@jackgerrits

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

It's tricky to know, I am targeting the last released version which was in 2016... It seems like we were on something like that as well in the source.

@lokitoth

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

We may want to do a manual test pass around good and bad input, and/or make a smoke-test?

@jackgerrits

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

A few of our tests go through json parsing, but yes there is potential for issues if behavior changed. It seems like versioning is not very good on Rapidjson but I think we are on the same major and minor version

@lokitoth

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

Okay, that seems reasonable. Expectation should be that if major/minor remain constant, behaviour should be the same up to bug fixes.

@jackgerrits

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Just diffed the submodule against the old checked in source and the only changes are whitespace as far as I can tell

jackgerrits and others added some commits Apr 11, 2019

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

We should merge as soon as tests pass.

@JohnLangford

This comment has been minimized.

jackgerrits added some commits Apr 29, 2019

@jackgerrits jackgerrits force-pushed the jackgerrits:jagerrit/remove_rapid_json branch from d931fac to 2ff6b3d Apr 29, 2019

jackgerrits added some commits Apr 30, 2019

@jackgerrits

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

The LGTM failure is a known issue and will be resolved when we merge the PR. See this forum post for details: https://discuss.lgtm.com/t/git-submodule-sync-missing/1989

jackgerrits added some commits May 1, 2019

@jackgerrits jackgerrits merged commit aeb90b0 into VowpalWabbit:master May 2, 2019

5 of 8 checks passed

LGTM analysis: C# Analysis failed (could not build the merge commit)
Details
LGTM analysis: C/C++ Analysis failed (could not build the merge commit)
Details
LGTM analysis: JavaScript Analysis failed (could not build the merge commit)
Details
LGTM analysis: Java No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 72.956%
Details

@jackgerrits jackgerrits deleted the jackgerrits:jagerrit/remove_rapid_json branch May 8, 2019

jackgerrits added a commit to jackgerrits/vowpal_wabbit that referenced this pull request May 15, 2019

Consume RapidJson through submodule (VowpalWabbit#1828)
* Move rapid json to a consumed dependency

* Remove rapidjson

* Update dockerfile definition

* move to consume through submodule

* Remove shallow_clone

* revert dockerfile change

* undo nuget change

* actually revert change

* Fix bad revert of vw_core.vcxproj

* Fix bad merge error

* Use absolute url because LGTM isnt working

* Revert "Use absolute url because LGTM isnt working"

This reverts commit 163cc08.
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.