Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8394002:
exclude the linux files that are not needed on openbsd (Closed)
|
Created:
9 years, 2 months ago by Robert Nagy Modified:
9 years, 2 months ago CC:
chromium-reviews, hclam+watch_chromium.org, cbentzel+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, acolwell+watch_chromium.org, jam, annacc+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptionexclude the linux files that are not needed on openbsd
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107185
Patch Set 1 #
Total comments: 12
Patch Set 2 : anchor #Patch Set 3 : fixes for prev. patchset #
Total comments: 4
Patch Set 4 : fixes for comments #
Total comments: 2
Patch Set 5 : use sources! #
Messages
Total messages: 16 (0 generated)
I did not create separate issues for these since they are really small and safe.
http://codereview.chromium.org/8394002/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/8394002/diff/1/content/content_browser.gypi#ne... content/content_browser.gypi:686: ['exclude', '^browser/geolocation/wifi_data_provider_linux.cc'], If you're anchoring with a ^ on one end, anchor with a $ on the other. Also escape (backslash) the dot. http://codereview.chromium.org/8394002/diff/1/content/content_plugin.gypi File content/content_plugin.gypi (right): http://codereview.chromium.org/8394002/diff/1/content/content_plugin.gypi#new... content/content_plugin.gypi:39: ['exclude', '^plugin/plugin_main_linux\\.cc'], Anchor with a $. http://codereview.chromium.org/8394002/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/8394002/diff/1/media/media.gyp#newcode324 media/media.gyp:324: ['exclude', 'pulse_' ], Should this match with a leading slash or something else? http://codereview.chromium.org/8394002/diff/1/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8394002/diff/1/net/net.gyp#newcode842 net/net.gyp:842: ['exclude', '^base/network_change_notifier_linux\\.cc'], $ on these too.
http://codereview.chromium.org/8394002/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/8394002/diff/1/content/content_browser.gypi#ne... content/content_browser.gypi:686: ['exclude', '^browser/geolocation/wifi_data_provider_linux.cc'], On 2011/10/25 15:03:04, Mark Mentovai wrote: > If you're anchoring with a ^ on one end, anchor with a $ on the other. Also > escape (backslash) the dot. I did not do this to follow what's been done in the file already. Should I change each and every exclude/include to reflect this? http://codereview.chromium.org/8394002/diff/1/content/content_plugin.gypi File content/content_plugin.gypi (right): http://codereview.chromium.org/8394002/diff/1/content/content_plugin.gypi#new... content/content_plugin.gypi:39: ['exclude', '^plugin/plugin_main_linux\\.cc'], On 2011/10/25 15:03:04, Mark Mentovai wrote: > Anchor with a $. Done. http://codereview.chromium.org/8394002/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/8394002/diff/1/media/media.gyp#newcode324 media/media.gyp:324: ['exclude', 'pulse_' ], On 2011/10/25 15:03:04, Mark Mentovai wrote: > Should this match with a leading slash or something else? Nope, this works fine. http://codereview.chromium.org/8394002/diff/1/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8394002/diff/1/net/net.gyp#newcode842 net/net.gyp:842: ['exclude', '^base/network_change_notifier_linux\\.cc'], On 2011/10/25 15:03:04, Mark Mentovai wrote: > $ on these too. Done.
http://codereview.chromium.org/8394002/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/8394002/diff/1/content/content_browser.gypi#ne... content/content_browser.gypi:686: ['exclude', '^browser/geolocation/wifi_data_provider_linux.cc'], Robert Nagy wrote: > I did not do this to follow what's been done in the file already. Should I > change each and every exclude/include to reflect this? This file is wrong. :) Why don’t you save fixing the rest of this file up for a subsequent change? http://codereview.chromium.org/8394002/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/8394002/diff/1/media/media.gyp#newcode324 media/media.gyp:324: ['exclude', 'pulse_' ], Robert Nagy wrote: > On 2011/10/25 15:03:04, Mark Mentovai wrote: > > Should this match with a leading slash or something else? > > Nope, this works fine. You misunderstood me. I’ll rephrase: This should match with a leading slash. So should the other two things here. http://codereview.chromium.org/8394002/diff/1/media/media.gyp#newcode325 media/media.gyp:325: ['exclude', '\\.mm?$' ] ], I’m surprised this is necessary, because I thought that the non-Mac builds excluded .m and .mm files automatically. I see no other platform needing to exclude them explicitly in this file.
http://codereview.chromium.org/8394002/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/8394002/diff/1/content/content_browser.gypi#ne... content/content_browser.gypi:686: ['exclude', '^browser/geolocation/wifi_data_provider_linux.cc'], On 2011/10/25 16:06:10, Mark Mentovai wrote: > Robert Nagy wrote: > > I did not do this to follow what's been done in the file already. Should I > > change each and every exclude/include to reflect this? > > This file is wrong. :) > > Why don’t you save fixing the rest of this file up for a subsequent change? So be it :)
LGTM. I believe you need a net owner LGTM to use the CQ on this one.
not sure why you added me, you don't need my l g t m! the OWNERS files in content are one-level down from the gypi files by design On Tue, Oct 25, 2011 at 8:01 AM, <robert.nagy@gmail.com> wrote: > Reviewers: scherkus, John Abd-El-Malek, wtc, Mark Mentovai, > > Message: > I did not create separate issues for these since they are really small and > safe. > > Description: > exclude the linux files that are not needed on openbsd > > > BUG= > TEST= > > > Please review this at http://codereview.chromium.**org/8394002/<http://codereview.chromium.org/8394... > > SVN Base: http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr... > > Affected files: > M content/content_browser.gypi > M content/content_plugin.gypi > M media/media.gyp > M net/net.gyp > > > Index: content/content_browser.gypi > diff --git a/content/content_browser.gypi b/content/content_browser.gypi > index 994aa3e14784dbf9beb85523174f05**44df17fcec..** > fbcdc2036b8a969afefb2563898e95**bb0b229e05 100644 > --- a/content/content_browser.gypi > +++ b/content/content_browser.gypi > @@ -681,6 +681,11 @@ > 'browser/renderer_host/gtk_**key_bindings_handler.h', > ], > }], > + ['OS=="openbsd"', { > + 'sources/': [ > + ['exclude', '^browser/geolocation/wifi_** > data_provider_linux.cc'], > + ], > + }], > ['touchui==1', { > 'sources/': [ > ['exclude', '^browser/renderer_host/gtk_**im_context_wrapper.cc'], > Index: content/content_plugin.gypi > diff --git a/content/content_plugin.gypi b/content/content_plugin.gypi > index fd7d3f4999769c11d3be40f31a6d5f**1bbeecc486..** > d22a66868cd6f276f10773177a6d15**da632b841b 100644 > --- a/content/content_plugin.gypi > +++ b/content/content_plugin.gypi > @@ -34,6 +34,11 @@ > # These are layered in conditionals in the event other platforms > # end up using this module as well. > 'conditions': [ > + ['OS=="openbsd"', { > + 'sources/': [ > + ['exclude', '^plugin/plugin_main_linux\\.**cc'], > + ], > + }], > ['OS=="win"', { > 'include_dirs': [ > '<(DEPTH)/third_party/wtl/**include', > Index: media/media.gyp > diff --git a/media/media.gyp b/media/media.gyp > index 0f620000db4dca331d52b315f35b0b**656f7ecf69..** > d46d87f781aef7666bd235de2d8e23**72793abaac 100644 > --- a/media/media.gyp > +++ b/media/media.gyp > @@ -321,6 +321,7 @@ > ['OS=="openbsd"', { > 'sources/': [ ['exclude', 'alsa_' ], > ['exclude', 'audio_manager_linux' ], > + ['exclude', 'pulse_' ], > ['exclude', '\\.mm?$' ] ], > 'link_settings': { > 'libraries': [ > Index: net/net.gyp > diff --git a/net/net.gyp b/net/net.gyp > index c2ead15b21c6c0212694f29ac519f2**6ba4e44b96..** > 0d7ac1ea34c7ae0fc2c815c7d99d08**8128023ad1 100644 > --- a/net/net.gyp > +++ b/net/net.gyp > @@ -742,11 +742,6 @@ > 'proxy/proxy_config_service_**linux.h', > ], > }], > - ['OS=="openbsd"', { > - 'sources': [ > - 'base/platform_mime_util_**linux.cc', > - ], > - }], > ['use_kerberos==1', { > 'defines': [ > 'USE_KERBEROS', > @@ -842,6 +837,12 @@ > 'dependencies': [ > '../build/linux/system.gyp:**libresolv', > ], > + },{ > + 'sources/': [ > + ['exclude', '^base/network_change_** > notifier_linux\\.cc'], > + ['exclude', '^base/network_change_** > notifier_netlink_linux\\.cc'], > + ['exclude', '^proxy/proxy_config_service_** > linux\\.cc'], > + ], > }], > ['OS=="solaris"', { > 'link_settings': { > > >
Review comments on Patch Set 3: http://codereview.chromium.org/8394002/diff/8001/net/net.gyp File net/net.gyp (left): http://codereview.chromium.org/8394002/diff/8001/net/net.gyp#oldcode747 net/net.gyp:747: 'base/platform_mime_util_linux.cc', Is base/platform_mime_util_linux.cc not suitable for OpenBSD? http://codereview.chromium.org/8394002/diff/8001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8394002/diff/8001/net/net.gyp#newcode836 net/net.gyp:836: ['OS!="openbsd"', { Please change the test to OS=="openbsd" and reverse the two consitional blocks. Is there a rule elsewhere to use _linux.cc files for OpenBSD by default? I don't understand why you need to exclude these _linux.cc files explicitly.
http://codereview.chromium.org/8394002/diff/8001/net/net.gyp File net/net.gyp (left): http://codereview.chromium.org/8394002/diff/8001/net/net.gyp#oldcode747 net/net.gyp:747: 'base/platform_mime_util_linux.cc', On 2011/10/25 17:50:31, wtc wrote: > > Is base/platform_mime_util_linux.cc not suitable for OpenBSD? We include all of the *_linux* files and directories since there are so many of them that there is no point in including them individually and after the discussion with Mark, the best solution is to include all of them and then exclude the ones that are not needed. This was commited in a separate patchset earlier this week. http://codereview.chromium.org/8394002/diff/8001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8394002/diff/8001/net/net.gyp#newcode836 net/net.gyp:836: ['OS!="openbsd"', { On 2011/10/25 17:50:31, wtc wrote: > > Please change the test to > OS=="openbsd" > and reverse the two consitional blocks. > > Is there a rule elsewhere to use _linux.cc files for OpenBSD > by default? I don't understand why you need to exclude these > _linux.cc files explicitly. Done. And yes, i've answered your question in the other file's comments.
media LGTM
Review comments on Patch Set 4: http://codereview.chromium.org/8394002/diff/4005/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8394002/diff/4005/net/net.gyp#newcode841 net/net.gyp:841: ], I'm sorry I didn't spot this problem earlier: please use the 'sources!' directive to exclude individual files, which does not require the use of ^, $, and "\\.". See lines 857-873 below for an example.
http://codereview.chromium.org/8394002/diff/4005/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8394002/diff/4005/net/net.gyp#newcode841 net/net.gyp:841: ], On 2011/10/25 18:28:41, wtc wrote: > > I'm sorry I didn't spot this problem earlier: please use the > 'sources!' directive to exclude individual files, which does > not require the use of ^, $, and "\\.". > > See lines 857-873 below for an example. Done.
Patch Set 5 LGTM. Thanks!
LGTM if it works for you on OpenBSD. Hitting the CQ.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8394002/8002
Change committed as 107185 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
