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

(LTH-130) Unvendor RapidJSON #289

Closed
wants to merge 3 commits into from
Closed

Conversation

smortex
Copy link
Contributor

@smortex smortex commented Jan 10, 2019

This is an attempt of extracting RapidJSON from the leatherman archive.

Work was started in #265 by @MikaelSmith but not finished. Take his work, rebase on top of master, add a fix and see what happens 😉

@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@smortex smortex force-pushed the rapidjson-1.1.0 branch 23 times, most recently from ea32367 to d83c2a2 Compare January 11, 2019 03:39
@smortex
Copy link
Contributor Author

smortex commented Jan 11, 2019

Yay! After a few hours of pain, it's all green 😌

@MikaelSmith, because of divergences, I had to change the way RapidJSON is made available during the rebase. For build on UNIX-like systems, I chose to install the rapidjson-dev package which contains all required parts:

apk add --no-cache rapidjson-dev &&

For windows, I chose to grab the source code, and use the "regular" (?) build tools to do what I think is equivalent to ./configure && make && make install:

leatherman/appveyor.yml

Lines 16 to 23 in 73f08a5

cd C:\projects
curl --location -o v1.1.0.zip https://github.com/Tencent/rapidjson/archive/v1.1.0.zip
7z.exe x v1.1.0.zip
cd rapidjson-1.1.0
cmake .
msbuild /P:Configuration=Release RapidJSON.sln
msbuild /P:Configuration=Release INSTALL.vcxproj
cd C:\projects\leatherman

Please tell me if you mind, and I'll try to amend this commit to separate what you wrote from what I wrote.

The other changes are I guess straightforward.

This is ready for review, and could replace #265 and #287!

@smortex smortex changed the title [WIP] (LTH-130) Use RapidJSON 1.1.0 (LTH-130) Use RapidJSON 1.1.0 Jan 11, 2019
appveyor.yml Outdated Show resolved Hide resolved
MikaelSmith and others added 3 commits January 10, 2019 18:05
rapidjson is no longer vendored, it must be installed on the system
independently.
add_leatherman_includes() is a wrapper around CMake's list() function
which should be called with individual strings and not a single string.
@puppetcla
Copy link

CLA signed by all contributors.

@MikaelSmith
Copy link
Contributor

These changes make sense. They have some implications for downstream users. I think we (Puppet) will want to add a pre-compiled rapidjson to our myget feed.

@smortex
Copy link
Contributor Author

smortex commented Jan 11, 2019

I think we (Puppet) will want to add a pre-compiled rapidjson to our myget feed.

That look like a good idea, however, I don't know how to help to make this happen.

Similarly, adding rapidjson-dev directly to the docker image used may make sense. I found what I suspect to be the Dockerfile of this image yesterday, if you think it makes sense, I can open a Pull-Request to do so?

Thanks!

@smortex smortex changed the title (LTH-130) Use RapidJSON 1.1.0 (LTH-130) Use the system's version of RapidJSON Jan 13, 2019
@smortex smortex changed the title (LTH-130) Use the system's version of RapidJSON (LTH-130) Unvendor RapidJSON Jan 13, 2019
@Dorin-Pleava Dorin-Pleava changed the base branch from master to main December 14, 2020 12:07
@Dorin-Pleava Dorin-Pleava requested a review from a team December 14, 2020 12:07
@CLAassistant
Copy link

CLAassistant commented Sep 1, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ smortex
❌ MikaelSmith
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants