Add jsoncpp in DEPS and the gyp file to build jsoncpp in chromium.

Add jsoncpp in DEPS and the gyp file to build jsoncpp in chromium.

The jsoncpp is needed by libjingle as part of the webrtc effort. This patch probably not be the best solution as there's already one chrome json parser in base and another jsoncpp copy in native_client. However since the libjingle can depend on neither of them, here comes this patch. We hope to take this patch as a chance to get some advices on how should we handle this case.

BUGS=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106424

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106815

[Mark Larson, 2011-10-13 03:16:24]

From a licensing point of view, this is fine. That's about as far as I feel
qualified to review.

I don't think I'm the right person to ask for the right technical solution here.
It seems wrong to include another JSON parser, wrong to make every checkout
depend on something needed by libjingle, and wrong that libjingle can't depend
on this code itself. 

You should poke chromium-dev if you're really in a bind about how to solve the
dependency mess.

[Niklas Enbom, 2011-10-13 07:34:57]

Some background: NaCl already uses jsoncpp, but they have a local copy of the
code instead of a DEPS entry. Now when libJingle also needs jsoncpp we thought
it would be the right move to add the project to Chromium DEPS. Ideally NaCl
should do a CL that just switches their use of a copy to a DEPS, but they don't
have time to do that right away. 

There is a discussion on chromium-dev about this (subject jsoncpp), but as many
email threads it spun off in a different direction, and we thought commenting
around a CL would keep the discussion more on topic.

On 2011/10/13 03:16:24, Mark Larson wrote:
> From a licensing point of view, this is fine. That's about as far as I feel
> qualified to review.
> 
> I don't think I'm the right person to ask for the right technical solution
here.
> It seems wrong to include another JSON parser, wrong to make every checkout
> depend on something needed by libjingle, and wrong that libjingle can't depend
> on this code itself. 
> 
> You should poke chromium-dev if you're really in a bind about how to solve the
> dependency mess.

[Mark Larson, 2011-10-13 20:03:57]

LGTM

I see. I missed that thread. Apologies for that.

Before you make this live, we should have an internal SVN mirror. Please file a
ticket at http://goto/chromeinfraticket to request an SVN mirror for the
sourceforge repo.

If we don't do this, then release builders will fail on publishing the public
release DEPS file.

[Ronghua, 2011-10-13 21:28:41]

Thanks Mark. I created a ticket
http://code.google.com/p/chromium/issues/detail?id=100250 for the SVN
mirror.

On Thu, Oct 13, 2011 at 1:03 PM,  <mal@chromium.org> wrote:
> LGTM
>
> I see. I missed that thread. Apologies for that.
>
> Before you make this live, we should have an internal SVN mirror. Please
> file a
> ticket at http://goto/chromeinfraticket to request an SVN mirror for the
> sourceforge repo.
>
> If we don't do this, then release builders will fail on publishing the
> public
> release DEPS file.
>
> http://codereview.chromium.org/8251003/
>

[Ronghua, 2011-10-17 23:39:18]

The svn mirror has been created. Update the DEPS to use the mirror.

The only concern is with this way, both the external/internal users will use the
svn mirror which will increase the mirror server load.

If this is not acceptable, one alternative is to have something like this:

jsoncpp_repo = (Var("googlecode_url") % "jsoncpp")
if Var("googlecode_url") == "http://%s.googlecode.com/svn
  jsoncpp_repo = "http://jsoncpp.svn.sourceforge.net/svnroot/jsoncpp"


Please see more information in this ticket:
http://code.google.com/p/chromium/issues/detail?id=100250

[Nicolas Sylvain, 2011-10-18 00:31:37]

On Mon, Oct 17, 2011 at 4:39 PM, <ronghuawu@google.com> wrote:

> The svn mirror has been created. Update the DEPS to use the mirror.
>
> The only concern is with this way, both the external/internal users will
> use the
> svn mirror which will increase the mirror server load.
>
> If this is not acceptable, one alternative is to have something like this:
>
> jsoncpp_repo = (Var("googlecode_url") % "jsoncpp")
> if Var("googlecode_url") == "http://%s.googlecode.com/svn
>  jsoncpp_repo =
"http://jsoncpp.svn.**sourceforge.net/svnroot/**jsoncpp<http://jsoncpp.svn.sourceforge.net/svnroot/jsoncpp>
> "
>

Actually, can you just create a sourceforge_url at the top and use that?
I'll make sure the buildbot knows
how to translate it.

Thanks


>
>
> Please see more information in this ticket:
>
http://code.google.com/p/**chromium/issues/detail?id=**100250<http://code.goo...
>
>
http://codereview.chromium.**org/8251003/<http://codereview.chromium.org/8251...
>

[Ronghua, 2011-10-18 22:17:40]

Thanks Nicolas, that sounds like a great solution.

Added sourceforge_url.

[Nicolas Sylvain, 2011-10-18 22:33:47]

On Tue, Oct 18, 2011 at 3:17 PM, <ronghuawu@google.com> wrote:

> Thanks Nicolas, that sounds like a great solution.
>
> Added sourceforge_url.
>
>
>
http://codereview.chromium.**org/8251003/<http://codereview.chromium.org/8251...


Great, thanks.

The buildbot haven't been restarted yet with the fix.  How long can you sit
on this patch?
What about one more day?

Thanks

[nsylvain, 2011-10-18 22:41:46]

http://codereview.chromium.org/8251003/diff/16001/DEPS
File DEPS (right):

http://codereview.chromium.org/8251003/diff/16001/DEPS#newcode5
DEPS:5: "sourceforge_url": "https://%s.svn.sourceforge.net/svnroot",
can you change this to :
https://%(repo)s.svn.sourceforge.net/svnroot

And does it need to be https? can this be http for now, to be consistent with
the rest? (some machines have a hard time fetching from https)

http://codereview.chromium.org/8251003/diff/16001/DEPS#newcode279
DEPS:279: (Var("sourceforge_url") % "jsoncpp") + "/jsoncpp/trunk/jsoncpp@246",
and here:
% {"repo": "jsoncpp"}

[Ronghua, 2011-10-18 23:31:31]

Discussed with Nicolas offline and updated accordingly.

http://codereview.chromium.org/8251003/diff/16001/DEPS
File DEPS (right):

http://codereview.chromium.org/8251003/diff/16001/DEPS#newcode5
DEPS:5: "sourceforge_url": "https://%s.svn.sourceforge.net/svnroot",
On 2011/10/18 22:41:46, nsylvain wrote:
> can you change this to :
> https://%25%28repo%29s.svn.sourceforge.net/svnroot
> 
> And does it need to be https? can this be http for now, to be consistent with
> the rest? (some machines have a hard time fetching from https)

Done.

http://codereview.chromium.org/8251003/diff/16001/DEPS#newcode279
DEPS:279: (Var("sourceforge_url") % "jsoncpp") + "/jsoncpp/trunk/jsoncpp@246",
On 2011/10/18 22:41:46, nsylvain wrote:
> and here:
> % {"repo": "jsoncpp"}

Done.

[nsylvain, 2011-10-18 23:32:47]

I looked only at DEPS and it looks good. Try running a cron job though.

[Ronghua, 2011-10-18 23:45:18]

Merged with latest.

Nicolas, I will hold this change until you let me know the buildbot is ready.

Mark, do you want to take another look with the new changes?

[Mark Larson, 2011-10-19 00:25:37]

LGTM

On Tue, Oct 18, 2011 at 16:45, <ronghuawu@google.com> wrote:

> Merged with latest.
>
> Nicolas, I will hold this change until you let me know the buildbot is
> ready.
>
> Mark, do you want to take another look with the new changes?
>
>
>
http://codereview.chromium.**org/8251003/<http://codereview.chromium.org/8251...
>

[commit-bot: I haz the power, 2011-10-19 23:10:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@google.com/8251003/21006

[commit-bot: I haz the power, 2011-10-20 00:51:05]

Change committed as 106424

[Nico, 2011-10-20 02:52:44]

Please follow http://www.chromium.org/developers/adding-3rd-party-libraries when
adding 3rd-party code. In particular:

* Add a LICENSE file
* licenses.py / checklicenses.py
* Email open-source-third-party-reviews@google.com

[Ryan Sleevi, 2011-10-20 03:20:49]

On 2011/10/20 02:52:44, Nico wrote:
> Please follow http://www.chromium.org/developers/adding-3rd-party-libraries
when
> adding 3rd-party code. In particular:
> 
> * Add a LICENSE file
> * licenses.py / checklicenses.py
> * Email mailto:open-source-third-party-reviews@google.com

Filed http://crbug.com/100986 for the checklicenses.py failure and landed
http://crrev.com/106446 as a /temporary/ solution for the failure. Ideally this
could be fixed before re-landing (work with phajdan.jr if there are concerns)

[Ronghua, 2011-10-20 04:24:41]

Nicolas,

The cl has been reverted due to this error - "svn: PROPFIND of
'/svnroot/jsoncpp/trunk/jsoncpp': Server certificate verification failed: issuer
is not trusted". Is this caused by the fact the mirror svn is not enabled for
the build bot?


Nico/Ryan,

With the http://crrev.com/106446, the checklicenses.py failure is gone. If I add
the LICENSE file (from jsoncpp), can I re-land this patch? Do I need to email
open-source-third-party-reviews@google.com for review?



On 2011/10/20 03:20:49, Ryan Sleevi wrote:
> On 2011/10/20 02:52:44, Nico wrote:
> > Please follow http://www.chromium.org/developers/adding-3rd-party-libraries
> when
> > adding 3rd-party code. In particular:
> > 
> > * Add a LICENSE file
> > * licenses.py / checklicenses.py
> > * Email mailto:open-source-third-party-reviews@google.com
> 
> Filed http://crbug.com/100986 for the checklicenses.py failure and landed
> http://crrev.com/106446 as a /temporary/ solution for the failure. Ideally
this
> could be fixed before re-landing (work with phajdan.jr if there are concerns)

[Nico, 2011-10-20 04:45:12]

> With the http://crrev.com/106446, the checklicenses.py failure is gone. If I
> add the LICENSE file (from jsoncpp), can I re-land this patch?

Probably. Ask phajdan-jr (on #chromium during PST day hours) to be sure.

> Do I need to email
> open-source-third-party-reviews@google.com for review?

If you haven't already, then yes.

[Daniel Berlin, 2011-10-20 05:32:24]

lgtm

LGTM

[Daniel Berlin, 2011-10-20 05:32:29]

lgtm

lgtm

LGTM

[Daniel Berlin, 2011-10-20 05:33:32]

On 2011/10/20 05:32:29, Daniel Berlin wrote:
> lgtm
> 
> lgtm
> 
> LGTM

Sigh, the button got triple clicked somehow.
It *really looks good*

[nsylvain, 2011-10-20 16:37:33]

On Wed, Oct 19, 2011 at 9:24 PM, <ronghuawu@google.com> wrote:

> Nicolas,
>
> The cl has been reverted due to this error - "svn: PROPFIND of
> '/svnroot/jsoncpp/trunk/**jsoncpp': Server certificate verification
> failed: issuer
> is not trusted". Is this caused by the fact the mirror svn is not enabled
> for
> the build bot?
>
Not really.

It's because it's HTTPS, and the first time you sync, you need to approve
the
certificate.  This won't work.  We can't add HTTPS deps at this time. We
should
just use HTTP.

The mirror is up, so next time the main waterfall wont burn, but not all
bots
use the mirror, so we still need to switch to http.


>
>
> Nico/Ryan,
>
> With the http://crrev.com/106446, the checklicenses.py failure is gone. If
> I add
> the LICENSE file (from jsoncpp), can I re-land this patch? Do I need to
> email
>
open-source-third-party-**reviews@google.com<open-source-third-party-reviews@...
review?
>
>
>
>
> On 2011/10/20 03:20:49, Ryan Sleevi wrote:
>
>> On 2011/10/20 02:52:44, Nico wrote:
>> > Please follow http://www.chromium.org/**developers/adding-3rd-party-**
>> libraries <http://www.chromium.org/developers/adding-3rd-party-libraries>
>> when
>> > adding 3rd-party code. In particular:
>> >
>> > * Add a LICENSE file
>> > * licenses.py / checklicenses.py
>> > * Email
mailto:open-source-third-**party-reviews@google.com<open-source-third-party-reviews@google.com>
>>
>
>  Filed http://crbug.com/100986 for the checklicenses.py failure and landed
>> http://crrev.com/106446 as a /temporary/ solution for the failure.
>> Ideally
>>
> this
>
>> could be fixed before re-landing (work with phajdan.jr if there are
>> concerns)
>>
>
>
>
>
http://codereview.chromium.**org/8251003/<http://codereview.chromium.org/8251...
>

[tony, 2011-10-20 20:53:57]

Random 2 cents:
Checking this into third_party for nacl to use seems fine.

I'm a bit worried about writing code for libjingle to depend on jsoncpp because
that will just make it harder to get rid of a duplicate json parser in the
future.

Before writing new code against jsoncpp, I would prefer that we made a decision
about which json parser we want.  Specifically, we should either (1) start
upstreaming the features we need to jsoncpp or (2) start moving base/json into a
standalone third_party library.

On 2011/10/13 07:34:57, niklase1 wrote:
> Some background: NaCl already uses jsoncpp, but they have a local copy of the
> code instead of a DEPS entry. Now when libJingle also needs jsoncpp we thought
> it would be the right move to add the project to Chromium DEPS. Ideally NaCl
> should do a CL that just switches their use of a copy to a DEPS, but they
don't
> have time to do that right away. 
> 
> There is a discussion on chromium-dev about this (subject jsoncpp), but as
many
> email threads it spun off in a different direction, and we thought commenting
> around a CL would keep the discussion more on topic.
> 
> On 2011/10/13 03:16:24, Mark Larson wrote:
> > From a licensing point of view, this is fine. That's about as far as I feel
> > qualified to review.
> > 
> > I don't think I'm the right person to ask for the right technical solution
> here.
> > It seems wrong to include another JSON parser, wrong to make every checkout
> > depend on something needed by libjingle, and wrong that libjingle can't
depend
> > on this code itself. 
> > 
> > You should poke chromium-dev if you're really in a bind about how to solve
the
> > dependency mess.

[Ronghua, 2011-10-20 23:43:06]

* Add the jsoncpp to svn ignore list.
* Changed to use http instead of https
* Updated the location of the License file.
* Got approval from open-source-third-party-reviews@google.com (Daniel Berlin).
* Passed "src/tools/licenses.py scan"

The only thing now is the checklicenses.py.

Paweł, what should we do to get this to pass the licenses check? Do we have to
upstream the changes to jsoncpp project? Or is there any other way to do this,
for example maybe a manually review?



Tony,

Thanks for the comments. The libjingle is already using the jsoncpp. So we are
not adding the new code here. There's a discussion in chromium-dev titled
"jsoncpp", my understanding is that it will be very hard to make the base/json a
standalone as it depends on the base::Value.

With the current situation, we hope to get this one in first (as it's not
introducing more dependence to the jsoncpp), so that it won't block the webrtc
progress. At the same time we can consider how to get rid of the duplicate json
parsers.

[tony, 2011-10-20 23:48:20]

On 2011/10/20 23:43:06, Ronghua wrote:
> Thanks for the comments. The libjingle is already using the jsoncpp. So we are
> not adding the new code here. There's a discussion in chromium-dev titled
> "jsoncpp", my understanding is that it will be very hard to make the base/json
a
> standalone as it depends on the base::Value.

I'm the one who suggested on the email thread to move base/json and base/values
into third_party.  You just move the two together.

You would probably have to also move base/third_party/dmg_fp and update some
code to use ICU directly, but it should be doable.

[Ronghua, 2011-10-21 05:29:45]

Updated the license check script so that it can recognize the jsoncpp license
header. The checklicenses.py is success now.

Paweł, please review the license check script.

[Paweł Hajdan Jr., 2011-10-21 06:53:30]

LGTM, and thank you for your work here. I appreciate that you've done that after
the unexpected landing trouble, and I apologize for this little extra work.

http://codereview.chromium.org/8251003/diff/32002/third_party/devscripts/lice...
File third_party/devscripts/licensecheck.pl (right):

http://codereview.chromium.org/8251003/diff/32002/third_party/devscripts/lice...
third_party/devscripts/licensecheck.pl:531: $license = "MIT license $license";
This is good, just please use "MIT/X11 (BSD like)", and please update the patch
in third_party/devscripts.

http://codereview.chromium.org/8251003/diff/32002/tools/checklicenses/checkli...
File tools/checklicenses/checklicenses.py (right):

http://codereview.chromium.org/8251003/diff/32002/tools/checklicenses/checkli...
tools/checklicenses/checklicenses.py:242: 'MIT license'
nit: I think this should say "MIT/X11 (BSD like)", for consistency with the
script, and then you don't need the path-specific whitelist.

[Ronghua, 2011-10-21 19:04:06]

Not sure how to update the chromium-1.patch. Do I need to get the original
version from the debian and diff? Any instructions I can follow?

http://codereview.chromium.org/8251003/diff/32002/third_party/devscripts/lice...
File third_party/devscripts/licensecheck.pl (right):

http://codereview.chromium.org/8251003/diff/32002/third_party/devscripts/lice...
third_party/devscripts/licensecheck.pl:531: $license = "MIT license $license";
On 2011/10/21 06:53:30, Paweł Hajdan Jr. wrote:
> This is good, just please use "MIT/X11 (BSD like)", and please update the
patch
> in third_party/devscripts.

Done.

http://codereview.chromium.org/8251003/diff/32002/tools/checklicenses/checkli...
File tools/checklicenses/checklicenses.py (right):

http://codereview.chromium.org/8251003/diff/32002/tools/checklicenses/checkli...
tools/checklicenses/checklicenses.py:242: 'MIT license'
On 2011/10/21 06:53:30, Paweł Hajdan Jr. wrote:
> nit: I think this should say "MIT/X11 (BSD like)", for consistency with the
> script, and then you don't need the path-specific whitelist.

Done.

[Paweł Hajdan Jr., 2011-10-21 20:17:57]

LGTM

On 2011/10/21 19:04:06, Ronghua wrote:
> Not sure how to update the chromium-1.patch. Do I need to get the original
> version from the debian and diff? Any instructions I can follow?

I sent you instructions by e-mail, you can reverse-apply the patch to get the
original file.

Anyway, if that'd cause you too much trouble, I can handle that and switch to a
system that makes it easier to update the file. Feel free to commit as-is. LGTM.
Thank you again for your work on this!

>
http://codereview.chromium.org/8251003/diff/32002/third_party/devscripts/lice...
> File third_party/devscripts/licensecheck.pl (right):
> 
>
http://codereview.chromium.org/8251003/diff/32002/third_party/devscripts/lice...
> third_party/devscripts/licensecheck.pl:531: $license = "MIT license $license";
> On 2011/10/21 06:53:30, Paweł Hajdan Jr. wrote:
> > This is good, just please use "MIT/X11 (BSD like)", and please update the
> patch
> > in third_party/devscripts.
> 
> Done.
> 
>
http://codereview.chromium.org/8251003/diff/32002/tools/checklicenses/checkli...
> File tools/checklicenses/checklicenses.py (right):
> 
>
http://codereview.chromium.org/8251003/diff/32002/tools/checklicenses/checkli...
> tools/checklicenses/checklicenses.py:242: 'MIT license'
> On 2011/10/21 06:53:30, Paweł Hajdan Jr. wrote:
> > nit: I think this should say "MIT/X11 (BSD like)", for consistency with the
> > script, and then you don't need the path-specific whitelist.
> 
> Done.

[Ronghua, 2011-10-21 21:25:56]

Thanks Paweł. The chromium-1.patch is updated and I also add the
licensecheck.pl.orig.



I will send this patch to commit bot in half an hour if not objection.

[commit-bot: I haz the power, 2011-10-21 22:04:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@google.com/8251003/41002

[commit-bot: I haz the power, 2011-10-21 22:04:27]

Can't apply patch for file DEPS.
While running patch -p0 --forward --force;
patching file DEPS
Hunk #1 succeeded at 2 with fuzz 1.
Hunk #2 FAILED at 33.
1 out of 3 hunks FAILED -- saving rejects to file DEPS.rej

[commit-bot: I haz the power, 2011-10-21 22:22:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@google.com/8251003/41003

[commit-bot: I haz the power, 2011-10-22 00:33:20]

Change committed as 106815