Clean up gyp settings for libjpeg

Clean up gyp settings for libjpeg

The gyp settings for libjpeg currently have a number of problems ...
- Settings for using the system libjpeg are replicated in the gyp files for both libjpeg and libjpeg_turbo.
- The default setting for use_system_libjpeg is different depending on whether libjpeg or libjpeg_turbo is in use. This is unintuitive, as libjpeg_turbo is a drop-in replacement for libjpeg.

This patch improves the situation ...
- Improve documentation about the interaction between use_system_libjpeg and use_libjpeg_turbo.
- Use the libjpeg gyp file whenever use_system_libjpeg==1. This means that the settings for using the system libjpeg are only needed here. (A later patch will remove them from libjpeg_turbo's gyp file). It also means that the libjpeg_turbo repository can be excluded when use_system_libjpeg==1.
- Move all settings concerned with use_system_libjpeg to the libjpeg gyp file.

Note that with this change, we lose the OS-specific defaults for use_system_libjpeg when use_libjpeg_turbo==0. These defaults are currently not used, as use_libjpeg_turbo==1 always.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=147254

[Evan Martin, 2012-07-16 16:26:24]

Sorry to disappear now, but I don't officially work on Chrome anymore and
this is starting to get hard to follow.  It seems your intent is in the
right place, so perhaps you could find someone who understands libjpeg and
gyp to review it?


On Fri, Jul 13, 2012 at 7:37 AM, <steveblock@chromium.org> wrote:

> Reviewers: Evan Martin,
>
> Description:
> Clean up gyp settings for libjpeg
>
> The gyp settings for libjpeg currently have a number of problems ...
> - Settings for using the system libjpeg are replicated in the gyp files
> for both
> libjpeg and libjpeg_turbo.
> - The default setting for use_system_libjpeg is different depending on
> whether
> libjpeg or libjpeg_turbo is in use. This is unintuitive, as libjpeg_turbo
> is a
> drop-in replacement for libjpeg.
>
> This patch improves the situation ...
> - Improve documentation about the interaction between use_system_libjpeg
> and
> use_libjpeg_turbo.
> - Use the libjpeg gyp file whenever use_system_libjpeg==1. This means that
> the
> settings for using the system libjpeg are only needed here. (A later patch
> will
> remove them from libjpeg_turbo's gyp file). It also means that the
> libjpeg_turbo
> repository can be excluded when use_system_libjpeg==1.
> - Move all settings concerned with use_system_libjpeg to the libjpeg gyp
> file.
>
> Note that with this change, we lose the OS-specific defaults for
> use_system_libjpeg when use_libjpeg_turbo==0. These defaults are currently
> not
> used, as use_libjpeg_turbo==1 always.
>
> Please review this at
http://codereview.chromium.**org/10696082/<http://codereview.chromium.org/106...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M build/common.gypi
>   M third_party/iccjpeg/iccjpeg.**gyp
>   M third_party/libjpeg/libjpeg.**gyp
>   M ui/ui.gyp
>
>
> Index: build/common.gypi
> diff --git a/build/common.gypi b/build/common.gypi
> index efe1dc9cfb5e4f2a5ab59536db58e5**32aada23a4..**
> 73eb4864e58adc20eb71f41d151943**7fc92134d4 100644
> --- a/build/common.gypi
> +++ b/build/common.gypi
> @@ -183,6 +183,10 @@
>        # Use libjpeg-turbo as the JPEG codec used by Chromium.
>        'use_libjpeg_turbo%': 1,
>
> +      # Use system libjpeg. Note that the system's libjepg will be used
> even if
> +      # use_libjpeg_turbo is set.
> +      'use_system_libjpeg%': 0,
> +
>        # Variable 'component' is for cases where we would like to build
> some
>        # components as dynamic shared libraries but still need variable
>        # 'library' for static libraries.
> @@ -1058,11 +1062,12 @@
>
>        # Set the relative path from this file to the GYP file of the JPEG
>        # library used by Chromium.
> -      ['use_libjpeg_turbo==1', {
> -        'libjpeg_gyp_path': '../third_party/libjpeg_turbo/**libjpeg.gyp',
> -      }, {
> +      ['use_system_libjpeg==1 or use_libjpeg_turbo==0', {
> +        # Configuration for using the system libjeg is here.
>          'libjpeg_gyp_path': '../third_party/libjpeg/**libjpeg.gyp',
> -      }],  # use_libjpeg_turbo==1
> +      }, {
> +        'libjpeg_gyp_path': '../third_party/libjpeg_turbo/**libjpeg.gyp',
> +      }],
>
>        # Options controlling the use of GConf (the classic GNOME
> configuration
>        # system) and GIO, which contains GSettings (the new GNOME config
> system).
> Index: third_party/iccjpeg/iccjpeg.**gyp
> diff --git a/third_party/iccjpeg/iccjpeg.**gyp
> b/third_party/iccjpeg/iccjpeg.**gyp
> index 25d25edd06938b65ff9ca519cf9b8b**d67da3220a..**
> 780114858afb8778d8874f251f0a4b**98aa41ebbe 100644
> --- a/third_party/iccjpeg/iccjpeg.**gyp
> +++ b/third_party/iccjpeg/iccjpeg.**gyp
> @@ -14,13 +14,6 @@
>          'iccjpeg.c',
>          'iccjpeg.h',
>        ],
> -      'conditions': [
> -        ['os_bsd==1 and use_system_libjpeg==1', {
> -          'include_dirs': [
> -            '/usr/local/include',
> -          ],
> -        }],
> -      ],
>        'direct_dependent_settings': {
>          'include_dirs': [
>            '.',
> Index: third_party/libjpeg/libjpeg.**gyp
> diff --git a/third_party/libjpeg/libjpeg.**gyp
> b/third_party/libjpeg/libjpeg.**gyp
> index 12cc882310cc5fbf9f3b2718854ef1**5bca606579..**
> 8f0dac82645fb45ed10a92656fc058**25f4da470f 100644
> --- a/third_party/libjpeg/libjpeg.**gyp
> +++ b/third_party/libjpeg/libjpeg.**gyp
> @@ -3,16 +3,8 @@
>  # found in the LICENSE file.
>
>  {
> -  'variables': {
> -    'conditions': [
> -      [ 'os_posix == 1 and OS != "mac"', {
> -        # Link to system .so since we already use it due to GTK.
> -        'use_system_libjpeg%': 1,
> -      }, {  # os_posix != 1 or OS == "mac"
> -        'use_system_libjpeg%': 0,
> -      }],
> -    ],
> -  },
> +  # This file handles building both with our local libjpeg and with the
> system
> +  # libjpeg.
>    'conditions': [
>      ['use_system_libjpeg==0', {
>        'targets': [
> @@ -94,6 +86,13 @@
>              'defines': [
>                'USE_SYSTEM_LIBJPEG',
>              ],
> +            'conditions': [
> +              ['os_bsd==1', {
> +                'include_dirs': [
> +                  '/usr/local/include',
> +                ],
> +              }],
> +            ],
>            },
>            'link_settings': {
>              'libraries': [
> Index: ui/ui.gyp
> diff --git a/ui/ui.gyp b/ui/ui.gyp
> index ab7a4a127f83f7e1421f12c6beec7a**776ec705f4..**
> c51550ccfc95492caf38f72b85241a**2623c64955 100644
> --- a/ui/ui.gyp
> +++ b/ui/ui.gyp
> @@ -704,11 +704,6 @@
>              '-ldl',
>            ],
>          }],
> -        ['os_bsd==1 and use_system_libjpeg==1', {
> -          'include_dirs': [
> -            '/usr/local/include',
> -          ],
> -        }],
>          ['inside_chromium_build==0', {
>            'dependencies': [
>              '<(DEPTH)/webkit/support/**setup_third_party.gyp:third_**
> party_headers',
>
>
>

[Steve Block, 2012-07-17 13:50:30]

Removing Evan as he no longer works on Chromium.

This change is the result of the discussion on
http://codereview.chromium.org/10699059

[Hironori Bono, 2012-07-18 00:01:58]

Greetings,

Thanks for your change.
LGTM (for libjpeg.gyp). It seems your change also needs an approval of a UI
owner (ben or sky), though.

Regards,

Hironori Bono

[Steve Block, 2012-07-18 09:35:46]

Thanks

+ben and sky for ui/ OWNERS

[sky, 2012-07-18 15:17:54]

LGTM

[commit-bot: I haz the power, 2012-07-18 15:51:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/10696082/4003

[commit-bot: I haz the power, 2012-07-18 16:54:13]

Try job failure for 10696082-4003 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Steve Block, 2012-07-18 17:13:36]

Cleanup change to libjpeg_turbo is waiting for review at
http://codereview.chromium.org/10777011

[Nico, 2012-07-19 19:36:17]

+phajdan, who cares about the use_system_ stuff.

[Paweł Hajdan Jr., 2012-08-29 14:48:40]

Thank you for including me. This looks good (LGTM). I appreciate your effort to
keep use_system_ flags working and cleanly implemented.