Created BUILD.gn for libsecret

Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.

Q: Why move the headers in a subdirectory?
A: Dependents of libsecret expect the headers as <libsecret/secret.h> (libsecret files also reference each other this way). The previous way this worked was by including the entire //third_party/. This is too broad and it is also not configured by libsecret itself. Rather, libsecret piggybacks on other components doing it.
Moving the headers into "//third_party/libsecret/libsecret/" allows us to include "//third_party/libsecret/" and keep referencing the headers like previously. It also removes the situation where libsecret piggybacks on other libraries, or that other libraries will piggyback on libsecret.

BUG=606303
Committed: https://crrev.com/7267be8dae266d389392b211c5746fd1192a0ee5
Cr-Commit-Position: refs/heads/master@{#390062}

[cfroussios, 2016-04-26 13:49:12]

Description was changed from

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.


BUG=606303
==========

to

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.


BUG=606303
==========

[cfroussios, 2016-04-26 13:49:12]

cfroussios@chromium.org changed reviewers:
+ vabr@chromium.org

[cfroussios, 2016-04-26 13:51:45]

On 2016/04/26 13:49:12, cfroussios wrote:
> mailto:cfroussios@chromium.org changed reviewers:
> + mailto:vabr@chromium.org

Hey vabr. Can you have a look at why specific linux bots are failing to compile?

[vabr (Chromium), 2016-04-27 07:10:12]

The failure looks like some troubles of the trybot, so I am rerunning it. Let's
see how that works out.

https://codereview.chromium.org/1922563002/diff/80001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1922563002/diff/80001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2557: 'cflags' : [
It looks like 'include_dirs' is used instead of 'cflags' for this.

[vabr (Chromium), 2016-04-27 08:04:44]

On 2016/04/27 07:10:12, vabr (Chromium) wrote:
> The failure looks like some troubles of the trybot, so I am rerunning it.
Let's
> see how that works out.
> 
>
https://codereview.chromium.org/1922563002/diff/80001/chrome/chrome_tests_uni...
> File chrome/chrome_tests_unit.gypi (right):
> 
>
https://codereview.chromium.org/1922563002/diff/80001/chrome/chrome_tests_uni...
> chrome/chrome_tests_unit.gypi:2557: 'cflags' : [
> It looks like 'include_dirs' is used instead of 'cflags' for this.

The bot is now green, so it indeed was an unrelated glitch.

[vabr (Chromium), 2016-04-27 11:14:25]

Thanks, Christos!

Looking mostly good, some comments below.

Cheers,
Vaclav

https://codereview.chromium.org/1922563002/diff/100001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1922563002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3645: '../third_party/libsecret/'
Normally, I would say: similarly to how you did this with GN, it would also be
good to add a gyp file into third_party/libsecret, which would specify the
dependency on the headers and add the include directory.

But as on Linux GYP is being actively deprecated just now, I don't think it's
worth to bother with it any more.

https://codereview.chromium.org/1922563002/diff/100001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (left):

https://codereview.chromium.org/1922563002/diff/100001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:284:
'browser/ui/webui/local_state/local_state_ui_unittest.cc',
Did you delete this and the following files by accident?

https://codereview.chromium.org/1922563002/diff/100001/third_party/libsecret/...
File third_party/libsecret/BUILD.gn (right):

https://codereview.chromium.org/1922563002/diff/100001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:5: config("libsecret_as_system") {
nit: The "as_system" could be confusing. I guess it relates to using <> in the
#include? I'm not sure we need to mention this in the config name. The name can
be something generic, like "libsecret_config", because it is clear from the body
itself what it does.

https://codereview.chromium.org/1922563002/diff/100001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:9: component("libsecret") {
This could be a source_set. A component is a shared_library in component build,
and that does not make sense for a collection of headers. A source_set is simply
a set of sources. If there were any .cc files, those would get compiled, but not
linked into a library, but that's not applicable here anyway. For headers, there
is no action.

https://codereview.chromium.org/1922563002/diff/100001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:10: public_configs = [ ":libsecret_as_system" ]
Please also add the "sources" field and list all the headers, to capture the
dependency (otherwise changes to the headers won't result in recompilation, and
missing headers will be reported by the compiler instead of earlier by GN).

[cfroussios, 2016-04-27 12:44:02]

Hi, I applied your recommendations

https://codereview.chromium.org/1922563002/diff/100001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1922563002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3645: '../third_party/libsecret/'
On 2016/04/27 11:14:24, vabr (Chromium) wrote:
> Normally, I would say: similarly to how you did this with GN, it would also be
> good to add a gyp file into third_party/libsecret, which would specify the
> dependency on the headers and add the include directory.
> 
> But as on Linux GYP is being actively deprecated just now, I don't think it's
> worth to bother with it any more.

Acknowledged.

https://codereview.chromium.org/1922563002/diff/100001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (left):

https://codereview.chromium.org/1922563002/diff/100001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:284:
'browser/ui/webui/local_state/local_state_ui_unittest.cc',
On 2016/04/27 11:14:24, vabr (Chromium) wrote:
> Did you delete this and the following files by accident?

I was just noticing this happened. Fixing...

https://codereview.chromium.org/1922563002/diff/100001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:284:
'browser/ui/webui/local_state/local_state_ui_unittest.cc',
On 2016/04/27 11:14:24, vabr (Chromium) wrote:
> Did you delete this and the following files by accident?

Done.

https://codereview.chromium.org/1922563002/diff/100001/third_party/libsecret/...
File third_party/libsecret/BUILD.gn (right):

https://codereview.chromium.org/1922563002/diff/100001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:5: config("libsecret_as_system") {
On 2016/04/27 11:14:24, vabr (Chromium) wrote:
> nit: The "as_system" could be confusing. I guess it relates to using <> in the
> #include? I'm not sure we need to mention this in the config name. The name
can
> be something generic, like "libsecret_config", because it is clear from the
body
> itself what it does.

Done.

https://codereview.chromium.org/1922563002/diff/100001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:9: component("libsecret") {
On 2016/04/27 11:14:24, vabr (Chromium) wrote:
> This could be a source_set. A component is a shared_library in component
build,
> and that does not make sense for a collection of headers. A source_set is
simply
> a set of sources. If there were any .cc files, those would get compiled, but
not
> linked into a library, but that's not applicable here anyway. For headers,
there
> is no action.

Done.

https://codereview.chromium.org/1922563002/diff/100001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:10: public_configs = [ ":libsecret_as_system" ]
On 2016/04/27 11:14:24, vabr (Chromium) wrote:
> Please also add the "sources" field and list all the headers, to capture the
> dependency (otherwise changes to the headers won't result in recompilation,
and
> missing headers will be reported by the compiler instead of earlier by GN).

Done.

[vabr (Chromium), 2016-04-27 12:44:58]

LGTM once you address the one new comment below.

Thanks!
Vaclav

https://codereview.chromium.org/1922563002/diff/120001/third_party/libsecret/...
File third_party/libsecret/BUILD.gn (right):

https://codereview.chromium.org/1922563002/diff/120001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:11: "secret-attributes.h",
It looks like all the names should have the libsecret/ prefix. Did GN not
complain that secret-*.h is needed but there is no rule how to make it?

[cfroussios, 2016-04-27 13:04:03]

https://codereview.chromium.org/1922563002/diff/120001/third_party/libsecret/...
File third_party/libsecret/BUILD.gn (right):

https://codereview.chromium.org/1922563002/diff/120001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:11: "secret-attributes.h",
On 2016/04/27 12:44:58, vabr (Chromium) wrote:
> It looks like all the names should have the libsecret/ prefix. Did GN not
> complain that secret-*.h is needed but there is no rule how to make it?

No, it worked like there's no problem. Does that mean that there's something
else missing?

https://codereview.chromium.org/1922563002/diff/120001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:11: "secret-attributes.h",
On 2016/04/27 12:44:58, vabr (Chromium) wrote:
> It looks like all the names should have the libsecret/ prefix. Did GN not
> complain that secret-*.h is needed but there is no rule how to make it?

Done.

[cfroussios, 2016-04-27 13:19:30]

cfroussios@chromium.org changed reviewers:
+ jochen@chromium.org

[cfroussios, 2016-04-27 13:19:31]

jochen@chromium.org: Please review changes in chrome/browser/BUILD.gn

[jochen (gone - plz use gerrit), 2016-04-27 13:22:37]

can you explain why you move the headers?

would it make sense to split this from adding the gn file?

[vabr (Chromium), 2016-04-27 13:29:43]

On 2016/04/27 13:22:37, jochen wrote:
> can you explain why you move the headers?
> 
> would it make sense to split this from adding the gn file?

The headers reference each other as <libsecret/*.h>. To make this work
currently, they need the include path to involve "third_party/", which seems a
bit broad. It happens to be set for chrome, but not for some components, for
example.
Moving them to libsecret, i.e., having them as
third_party/libsecret/libsecret/*.h allows to just add "third_party/libsecret"
to the include paths.

The need to specify the include paths (for properly using this in components,
which is going to be another CL) is the reason why we are adding the build
files, that's why it ended up in once CL. If you prefer to split that, I have no
strong objection.

Cheers,
Vaclav

[jochen (gone - plz use gerrit), 2016-04-27 13:32:12]

lgtm

[vabr (Chromium), 2016-04-27 13:36:16]

https://codereview.chromium.org/1922563002/diff/120001/third_party/libsecret/...
File third_party/libsecret/BUILD.gn (right):

https://codereview.chromium.org/1922563002/diff/120001/third_party/libsecret/...
third_party/libsecret/BUILD.gn:11: "secret-attributes.h",
On 2016/04/27 13:04:03, cfroussios wrote:
> On 2016/04/27 12:44:58, vabr (Chromium) wrote:
> > It looks like all the names should have the libsecret/ prefix. Did GN not
> > complain that secret-*.h is needed but there is no rule how to make it?
> 
> No, it worked like there's no problem. Does that mean that there's something
> else missing?

Interesting. I tried that out and it indeed seems that GN does not scream if *.h
is missing. It does scream if *.cc is missing. It also seems to be ignoring
changes to the header, which points out that I'm not understanding GN. I'll try
to investigate that, but in the meantime, having the sources listed still seems
to me like the right thing to do.

[vabr (Chromium), 2016-04-27 13:37:56]

Christos, before you land this, could you please consider also extending the CL
description to explain why we move the headers into the subdirectory. Jochen's
question was a good point and more people might be asking it even later.

Thanks!
Vaclav

[cfroussios, 2016-04-27 13:38:55]

The CQ bit was checked by cfroussios@chromium.org

[cfroussios, 2016-04-27 13:38:55]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1922563002/#ps140001
(title: "Corrected source set")

[cfroussios, 2016-04-27 13:39:07]

The CQ bit was unchecked by cfroussios@chromium.org

[cfroussios, 2016-04-27 13:51:43]

Description was changed from

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.


BUG=606303
==========

to

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.

Q: Why move the headers in a subdirectory?
A: Dependents of libsecret expect the headers as <libsecret/secret.h> (libsecret
files also reference each other this way). The previous way this worked was by
including the entire //third_party/. This is too broad and it is also not
required by libsecret itself. Rather, libsecret piggybacks on other components
doing it.
Moving the headers into "//third_party/libsecret/libsecret/" allows us to
include "//third_party/libsecret/" and keep referencing the headers like
previously. It also removes the situation where libsecret piggybacks on other
libraries, or that other libraries will piggyback on libsecret. 



BUG=606303
==========

[vabr (Chromium), 2016-04-27 13:52:49]

On 2016/04/27 13:36:16, vabr (Chromium) wrote:
>
https://codereview.chromium.org/1922563002/diff/120001/third_party/libsecret/...
> File third_party/libsecret/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1922563002/diff/120001/third_party/libsecret/...
> third_party/libsecret/BUILD.gn:11: "secret-attributes.h",
> On 2016/04/27 13:04:03, cfroussios wrote:
> > On 2016/04/27 12:44:58, vabr (Chromium) wrote:
> > > It looks like all the names should have the libsecret/ prefix. Did GN not
> > > complain that secret-*.h is needed but there is no rule how to make it?
> > 
> > No, it worked like there's no problem. Does that mean that there's something
> > else missing?
> 
> Interesting. I tried that out and it indeed seems that GN does not scream if
*.h
> is missing. It does scream if *.cc is missing. It also seems to be ignoring
> changes to the header, which points out that I'm not understanding GN. I'll
try
> to investigate that, but in the meantime, having the sources listed still
seems
> to me like the right thing to do.

I looked a bit more, and it seems the GN does care about headers, as long as
they are actually #included somewhere, even when they are #included with <>. But
it does not do anything for headers (unlike compilation for .cc), so it does not
even seem to check for the existence, relying on the compiler to detect it.
That's what I am observing, but I don't know how it is possible (to detect
change, but not absence). Anyway, the headers were present, so compiler did not
complain. I'll maybe post a question to chromium-dev, but nothing to worry about
here I guess.

[vabr (Chromium), 2016-04-27 13:53:35]

And the new CL description LGTM, thanks!

[cfroussios, 2016-04-27 13:53:48]

Description was changed from

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.

Q: Why move the headers in a subdirectory?
A: Dependents of libsecret expect the headers as <libsecret/secret.h> (libsecret
files also reference each other this way). The previous way this worked was by
including the entire //third_party/. This is too broad and it is also not
required by libsecret itself. Rather, libsecret piggybacks on other components
doing it.
Moving the headers into "//third_party/libsecret/libsecret/" allows us to
include "//third_party/libsecret/" and keep referencing the headers like
previously. It also removes the situation where libsecret piggybacks on other
libraries, or that other libraries will piggyback on libsecret. 



BUG=606303
==========

to

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.

Q: Why move the headers in a subdirectory?
A: Dependents of libsecret expect the headers as <libsecret/secret.h> (libsecret
files also reference each other this way). The previous way this worked was by
including the entire //third_party/. This is too broad and it is also not
configured by libsecret itself. Rather, libsecret piggybacks on other components
doing it.
Moving the headers into "//third_party/libsecret/libsecret/" allows us to
include "//third_party/libsecret/" and keep referencing the headers like
previously. It also removes the situation where libsecret piggybacks on other
libraries, or that other libraries will piggyback on libsecret. 



BUG=606303
==========

[cfroussios, 2016-04-27 13:54:04]

The CQ bit was checked by cfroussios@chromium.org

[commit-bot: I haz the power, 2016-04-27 13:54:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922563002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922563002/140001

[commit-bot: I haz the power, 2016-04-27 13:59:02]

Description was changed from

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.

Q: Why move the headers in a subdirectory?
A: Dependents of libsecret expect the headers as <libsecret/secret.h> (libsecret
files also reference each other this way). The previous way this worked was by
including the entire //third_party/. This is too broad and it is also not
configured by libsecret itself. Rather, libsecret piggybacks on other components
doing it.
Moving the headers into "//third_party/libsecret/libsecret/" allows us to
include "//third_party/libsecret/" and keep referencing the headers like
previously. It also removes the situation where libsecret piggybacks on other
libraries, or that other libraries will piggyback on libsecret. 



BUG=606303
==========

to

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.

Q: Why move the headers in a subdirectory?
A: Dependents of libsecret expect the headers as <libsecret/secret.h> (libsecret
files also reference each other this way). The previous way this worked was by
including the entire //third_party/. This is too broad and it is also not
configured by libsecret itself. Rather, libsecret piggybacks on other components
doing it.
Moving the headers into "//third_party/libsecret/libsecret/" allows us to
include "//third_party/libsecret/" and keep referencing the headers like
previously. It also removes the situation where libsecret piggybacks on other
libraries, or that other libraries will piggyback on libsecret. 



BUG=606303
==========

[commit-bot: I haz the power, 2016-04-27 13:59:03]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-27 14:00:45]

Description was changed from

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.

Q: Why move the headers in a subdirectory?
A: Dependents of libsecret expect the headers as <libsecret/secret.h> (libsecret
files also reference each other this way). The previous way this worked was by
including the entire //third_party/. This is too broad and it is also not
configured by libsecret itself. Rather, libsecret piggybacks on other components
doing it.
Moving the headers into "//third_party/libsecret/libsecret/" allows us to
include "//third_party/libsecret/" and keep referencing the headers like
previously. It also removes the situation where libsecret piggybacks on other
libraries, or that other libraries will piggyback on libsecret. 



BUG=606303
==========

to

==========
Created BUILD.gn for libsecret

Moved the libsecret headers to a subdirectory and added a config rule to include
this directory during compilation of dependents.

Also updated chrome/browser/BUILD.gn to use this dependency in the new way.

Q: Why move the headers in a subdirectory?
A: Dependents of libsecret expect the headers as <libsecret/secret.h> (libsecret
files also reference each other this way). The previous way this worked was by
including the entire //third_party/. This is too broad and it is also not
configured by libsecret itself. Rather, libsecret piggybacks on other components
doing it.
Moving the headers into "//third_party/libsecret/libsecret/" allows us to
include "//third_party/libsecret/" and keep referencing the headers like
previously. It also removes the situation where libsecret piggybacks on other
libraries, or that other libraries will piggyback on libsecret.

BUG=606303

Committed: https://crrev.com/7267be8dae266d389392b211c5746fd1192a0ee5
Cr-Commit-Position: refs/heads/master@{#390062}
==========

[commit-bot: I haz the power, 2016-04-27 14:00:46]

Patchset 8 (id:??) landed as
https://crrev.com/7267be8dae266d389392b211c5746fd1192a0ee5
Cr-Commit-Position: refs/heads/master@{#390062}