Fix Python imported modules depfile generation

Fix Python imported modules depfile generation

Some Python scripts incorrectly reported no python module deps, for
example a manual run of build/android/gn/zip.py would create an empty
depfile (without even a build_utils.py entry).

GetPythonDependencies assumed that CHROMIUM_SRC is an absolute path,
turns out that depends on details of how build_utils.py is imported.
If it happened to be relative, all modules would be omitted from the
depfile as "system" modules, as they did not match CHROMIUM_SRC. Fix by
using DIR_SOURCE_ROOT which is absolute (and assert that it is).

R=cjhopman@chromium.org, agrieve@chromium.org
BUG=359249
Committed: https://crrev.com/2e3ee5b02663f63194484fe5c4ec92c39a005617
Cr-Commit-Position: refs/heads/master@{#357246}

[tsniatowski, 2015-10-29 21:38:42]

PTAL. Slightly incorrect dependencies are never fun.

[agrieve, 2015-10-30 00:11:46]

https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
File build/android/gyp/util/build_utils.py (right):

https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_utils.py:341: abs_root =
os.path.abspath(CHROMIUM_SRC)
I think it'd be even better to make CHROMIUM_SRC an absolute path so that it
doesn't become invalid when the cwd changes

[agrieve, 2015-10-30 00:25:06]

https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
File build/android/gyp/util/build_utils.py (right):

https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_utils.py:341: abs_root =
os.path.abspath(CHROMIUM_SRC)
On 2015/10/30 00:11:45, agrieve wrote:
> I think it'd be even better to make CHROMIUM_SRC an absolute path so that it
> doesn't become invalid when the cwd changes

Actually - almost everywhere actually uses DIR_SOURCE_ROOT from:
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

Can you delete CHROMIUM_SRC in favour of DIR_SOURCE_ROOT? Seems only this file
and build/android/gyp/jinja_template.py use it.

[tsniatowski, 2015-10-30 00:31:21]

https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
File build/android/gyp/util/build_utils.py (right):

https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_utils.py:341: abs_root =
os.path.abspath(CHROMIUM_SRC)
On 2015/10/30 00:25:06, agrieve wrote:
> On 2015/10/30 00:11:45, agrieve wrote:
> > I think it'd be even better to make CHROMIUM_SRC an absolute path so that it
> > doesn't become invalid when the cwd changes
> 
> Actually - almost everywhere actually uses DIR_SOURCE_ROOT from:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
> 
> Can you delete CHROMIUM_SRC in favour of DIR_SOURCE_ROOT? Seems only this file
> and build/android/gyp/jinja_template.py use it.

The problem is when someone goes and changes it back to a relative path this
will silently break. I think since this function's correctness relies on
comparing absolute paths, it should explicitly make sure they are absolute.

DIR_SOURCE_ROOT seems okay too, I'll change it (tomorrow) [but personally I'd
keep the explicit abspath here, or an os.path.isabs check]

[agrieve, 2015-10-30 00:58:19]

On 2015/10/30 00:31:21, tsniatowski wrote:
>
https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
> File build/android/gyp/util/build_utils.py (right):
> 
>
https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
> build/android/gyp/util/build_utils.py:341: abs_root =
> os.path.abspath(CHROMIUM_SRC)
> On 2015/10/30 00:25:06, agrieve wrote:
> > On 2015/10/30 00:11:45, agrieve wrote:
> > > I think it'd be even better to make CHROMIUM_SRC an absolute path so that
it
> > > doesn't become invalid when the cwd changes
> > 
> > Actually - almost everywhere actually uses DIR_SOURCE_ROOT from:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
> > 
> > Can you delete CHROMIUM_SRC in favour of DIR_SOURCE_ROOT? Seems only this
file
> > and build/android/gyp/jinja_template.py use it.
> 
> The problem is when someone goes and changes it back to a relative path this
> will silently break. I think since this function's correctness relies on
> comparing absolute paths, it should explicitly make sure they are absolute.
> 
> DIR_SOURCE_ROOT seems okay too, I'll change it (tomorrow) [but personally I'd
> keep the explicit abspath here, or an os.path.isabs check]

I like the idea of adding an assert(). DIR_SOURCE_ROOT certainly is supposed to
always be absolute.

[tsniatowski, 2015-10-30 14:33:10]

Description was changed from

==========
Fix Python imported modules depfile generation

Some Python scripts incorrectly reported no python module deps, for
example a manual run of build/android/gn/zip.py would create an empty
depfile (without even a build_utils.py entry).

GetPythonDependencies assumed that CHROMIUM_SRC is an absolute path,
turns out that depends on details of how build_utils.py is imported.
If it happened to be relative, all modules would be omited from the
depfile as "system" modules, as they did not match CHROMIUM_SRC. Fix by
making sure the src path is absolute.

R=cjhopman@chromium.org, agrieve@chromium.org
BUG=359249
==========

to

==========
Fix Python imported modules depfile generation

Some Python scripts incorrectly reported no python module deps, for
example a manual run of build/android/gn/zip.py would create an empty
depfile (without even a build_utils.py entry).

GetPythonDependencies assumed that CHROMIUM_SRC is an absolute path,
turns out that depends on details of how build_utils.py is imported.
If it happened to be relative, all modules would be omitted from the
depfile as "system" modules, as they did not match CHROMIUM_SRC. Fix by
using DIR_SOURCE_ROOT which is absolute (and assert that it is).

R=cjhopman@chromium.org, agrieve@chromium.org
BUG=359249
==========

[agrieve, 2015-10-30 16:06:23]

On 2015/10/30 00:58:19, agrieve wrote:
> On 2015/10/30 00:31:21, tsniatowski wrote:
> >
>
https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
> > File build/android/gyp/util/build_utils.py (right):
> > 
> >
>
https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
> > build/android/gyp/util/build_utils.py:341: abs_root =
> > os.path.abspath(CHROMIUM_SRC)
> > On 2015/10/30 00:25:06, agrieve wrote:
> > > On 2015/10/30 00:11:45, agrieve wrote:
> > > > I think it'd be even better to make CHROMIUM_SRC an absolute path so
that
> it
> > > > doesn't become invalid when the cwd changes
> > > 
> > > Actually - almost everywhere actually uses DIR_SOURCE_ROOT from:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
> > > 
> > > Can you delete CHROMIUM_SRC in favour of DIR_SOURCE_ROOT? Seems only this
> file
> > > and build/android/gyp/jinja_template.py use it.
> > 
> > The problem is when someone goes and changes it back to a relative path this
> > will silently break. I think since this function's correctness relies on
> > comparing absolute paths, it should explicitly make sure they are absolute.
> > 
> > DIR_SOURCE_ROOT seems okay too, I'll change it (tomorrow) [but personally
I'd
> > keep the explicit abspath here, or an os.path.isabs check]
> 
> I like the idea of adding an assert(). DIR_SOURCE_ROOT certainly is supposed
to
> always be absolute.

lgtm

[agrieve, 2015-10-30 16:07:14]

On 2015/10/30 16:06:23, agrieve wrote:
> On 2015/10/30 00:58:19, agrieve wrote:
> > On 2015/10/30 00:31:21, tsniatowski wrote:
> > >
> >
>
https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
> > > File build/android/gyp/util/build_utils.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1412793012/diff/1/build/android/gyp/util/buil...
> > > build/android/gyp/util/build_utils.py:341: abs_root =
> > > os.path.abspath(CHROMIUM_SRC)
> > > On 2015/10/30 00:25:06, agrieve wrote:
> > > > On 2015/10/30 00:11:45, agrieve wrote:
> > > > > I think it'd be even better to make CHROMIUM_SRC an absolute path so
> that
> > it
> > > > > doesn't become invalid when the cwd changes
> > > > 
> > > > Actually - almost everywhere actually uses DIR_SOURCE_ROOT from:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
> > > > 
> > > > Can you delete CHROMIUM_SRC in favour of DIR_SOURCE_ROOT? Seems only
this
> > file
> > > > and build/android/gyp/jinja_template.py use it.
> > > 
> > > The problem is when someone goes and changes it back to a relative path
this
> > > will silently break. I think since this function's correctness relies on
> > > comparing absolute paths, it should explicitly make sure they are
absolute.
> > > 
> > > DIR_SOURCE_ROOT seems okay too, I'll change it (tomorrow) [but personally
> I'd
> > > keep the explicit abspath here, or an os.path.isabs check]
> > 
> > I like the idea of adding an assert(). DIR_SOURCE_ROOT certainly is supposed
> to
> > always be absolute.
> 
> lgtm

(fyi - you should always add a message after uploading a new patchset, as
uploading a patchset doesn't trigger an email to reviewers)

[tsniatowski, 2015-10-30 16:36:31]

The CQ bit was checked by tsniatowski@opera.com

[commit-bot: I haz the power, 2015-10-30 16:37:13]

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

[commit-bot: I haz the power, 2015-10-30 16:52:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 16:52:58]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[tsniatowski, 2015-10-30 23:36:30]

The CQ bit was checked by tsniatowski@opera.com

[tsniatowski, 2015-10-30 23:36:30]

The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/1412793012/#ps40001
(title: "Remove stray slash in os.path.join")

[commit-bot: I haz the power, 2015-10-30 23:37:20]

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

[commit-bot: I haz the power, 2015-10-31 00:59:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-31 01:02:53]

Patchset 3 (id:??) landed as
https://crrev.com/2e3ee5b02663f63194484fe5c4ec92c39a005617
Cr-Commit-Position: refs/heads/master@{#357246}