Point to build requirements page when there is a dependency check failure.

Point to build requirements page when there is a dependency check failure.

BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169952

[Peter Beverloo, 2014-03-20 15:09:12]

+dpranke FYI

lgtm. While I'm not entirely convinced this is the best way to put it, it does
beat overriding check_sys_deps and does add valuable information.

https://codereview.chromium.org/196413027/diff/1/Tools/Scripts/webkitpy/layou...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/196413027/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/port/base.py:386: if
sys.platform.startswith('linux'):
self.host.platform.is_linux()

https://codereview.chromium.org/196413027/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/port/base.py:388: _log.error('Did you
install the dependencies listed here?')
Please phrase this as a statement instead. To be consistent with Android, use:

"For complete Linux build requirements, please see:"

[mlamouri (slow - plz ping), 2014-03-20 17:21:01]

https://codereview.chromium.org/196413027/diff/1/Tools/Scripts/webkitpy/layou...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/196413027/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/port/base.py:386: if
sys.platform.startswith('linux'):
On 2014/03/20 15:09:12, Peter Beverloo wrote:
> self.host.platform.is_linux()

Done.

https://codereview.chromium.org/196413027/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/port/base.py:388: _log.error('Did you
install the dependencies listed here?')
On 2014/03/20 15:09:12, Peter Beverloo wrote:
> Please phrase this as a statement instead. To be consistent with Android, use:
> 
> "For complete Linux build requirements, please see:"

Done.

[mlamouri (slow - plz ping), 2014-03-20 17:21:04]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-20 17:21:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/196413027/20001

[commit-bot: I haz the power, 2014-03-20 17:55:51]

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

[commit-bot: I haz the power, 2014-03-20 17:55:51]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[Dirk Pranke, 2014-03-20 18:17:58]

This change does not look good to me as-is.

(though I like the idea and we should fix the problem).

https://codereview.chromium.org/196413027/diff/20001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/196413027/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/port/base.py:389:
_log.error('https://code.google.com/p/chromium/wiki/LayoutTestsLinux')

First, this is probably the wrong URL to link to; we should use
https://code.google.com/p/chromium/wiki/LinuxBuildInstructions instead, and
remove the bits about installing packages from LayoutTestsLinux altogether.

Second, I was troubled by the idea that we'd put a platform-specific check in
base.py (which we do sometimes, but I try to avoid), and also by the fact that
we're putting comments about build requirements in check_sys_deps() rather than
check_build(). If you look at win.py (for example), you'll see the same messages
in check_build() (with the Windows link, of course).

I think the right thing to do is to move this message into a
port._log_build_requirements_link() message, and refactor all of the port code
so that each port will log the right link if *either* check_sys_deps or
check_build() fails.

This shouldn't be very much work, and I think is the cleanest solution. Make
sense?

[Peter Beverloo, 2014-03-20 18:34:40]

https://codereview.chromium.org/196413027/diff/20001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/196413027/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/port/base.py:389:
_log.error('https://code.google.com/p/chromium/wiki/LayoutTestsLinux')
On 2014/03/20 18:17:58, Dirk Pranke wrote:
> 
> First, this is probably the wrong URL to link to; we should use
> https://code.google.com/p/chromium/wiki/LinuxBuildInstructions instead, and
> remove the bits about installing packages from LayoutTestsLinux altogether.

install-build-deps.sh indeed includes all the packages listed on that page, so
the normal build instructions should be sufficient.
 
> Second, I was troubled by the idea that we'd put a platform-specific check in
> base.py (which we do sometimes, but I try to avoid), and also by the fact that
> we're putting comments about build requirements in check_sys_deps() rather
than
> check_build(). If you look at win.py (for example), you'll see the same
messages
> in check_build() (with the Windows link, of course).
> 
> I think the right thing to do is to move this message into a
> port._log_build_requirements_link() message, and refactor all of the port code
> so that each port will log the right link if *either* check_sys_deps or
> check_build() fails.

There is some precedence for this; sorry. I do agree that sharing a URL to build
instructions if either of the methods fails is a lot better. With the more
granular return messages we now have this shouldn't be hard to do, either.

[Dirk Pranke, 2014-03-20 18:41:41]

On 2014/03/20 18:34:40, Peter Beverloo wrote:
> > Second, I was troubled by the idea that we'd put a platform-specific check
in
> > base.py (which we do sometimes, but I try to avoid), 

[ snip ]

> There is some precedence for this; sorry.

Yes, definitely there are precedents for port-specific code in base.py. 

I had to think for a while about *why* I didn't like it. I initially thought the
right thing to do was to subclass check_sys_deps() -- which you objected to --
and I realized that you were right that that would be worse, which is how I got
to where I ended up.

Dialectics in action!

[mlamouri (slow - plz ping), 2014-03-21 13:37:51]

On 2014/03/20 18:34:40, Peter Beverloo wrote:
>
https://codereview.chromium.org/196413027/diff/20001/Tools/Scripts/webkitpy/l...
> File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):
> 
>
https://codereview.chromium.org/196413027/diff/20001/Tools/Scripts/webkitpy/l...
> Tools/Scripts/webkitpy/layout_tests/port/base.py:389:
> _log.error('https://code.google.com/p/chromium/wiki/LayoutTestsLinux')
> On 2014/03/20 18:17:58, Dirk Pranke wrote:
> > 
> > First, this is probably the wrong URL to link to; we should use
> > https://code.google.com/p/chromium/wiki/LinuxBuildInstructions instead, and
> > remove the bits about installing packages from LayoutTestsLinux altogether.
> 
> install-build-deps.sh indeed includes all the packages listed on that page, so
> the normal build instructions should be sufficient.

The LayoutTest page points to install-build-deps.sh too. It seems to me that
LayoutTest specific information might be better but you both of you agree on
that, I will gladly change the URL. Let me know.

[Dirk Pranke, 2014-03-21 15:34:40]

On 2014/03/21 13:37:51, Mounir Lamouri wrote:
> The LayoutTest page points to install-build-deps.sh too. It seems to me that
> LayoutTest specific information might be better but you both of you agree on
> that, I will gladly change the URL. Let me know.

Right, I'm saying that we shouldn't document any LayoutTest-specific packages or
steps, we should just rely on the default, applies-to-everything instructions on
the other page (in which running install-build-deps.sh is a requirement).

I don't want to document a specific set of packages that are needed; such a list
will become out of date and we'll forget to update it.

[mlamouri (slow - plz ping), 2014-03-21 16:41:16]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-21 16:41:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/196413027/40001

[Peter Beverloo, 2014-03-21 16:45:39]

On 2014/03/21 16:41:26, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/mlamouri%40chromium.org/196413027/40001

not lgtm

The latest review comments have not been addressed, nor have they been replied
to. This patch should not have been put on the commit queue.

[Peter Beverloo, 2014-03-21 16:45:44]

The CQ bit was unchecked by peter@chromium.org

[mlamouri (slow - plz ping), 2014-03-21 17:04:13]

On 2014/03/21 16:45:39, Peter Beverloo wrote:
> On 2014/03/21 16:41:26, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://chromium-status.appspot.com/cq/mlamouri%2540chromium.org/196413027/40001
> 
> not lgtm
> 
> The latest review comments have not been addressed, nor have they been replied
> to. This patch should not have been put on the commit queue.

My understanding was that there were two comments: one about the URL, and one
about were the code is. I changed the URL. I thought that you guys agreed on
keeping the code as-is. Do you want me to introduce
port._log_build_requirements_link()?

[Dirk Pranke, 2014-03-21 18:10:57]

On 2014/03/21 17:04:13, Mounir Lamouri wrote:
> On 2014/03/21 16:45:39, Peter Beverloo wrote:
> > On 2014/03/21 16:41:26, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >
>
https://chromium-status.appspot.com/cq/mlamouri%252540chromium.org/196413027/...
> > 
> > not lgtm
> > 
> > The latest review comments have not been addressed, nor have they been
replied
> > to. This patch should not have been put on the commit queue.
> 
> My understanding was that there were two comments: one about the URL, and one
> about were the code is. I changed the URL. I thought that you guys agreed on
> keeping the code as-is. Do you want me to introduce
> port._log_build_requirements_link()?

Yes. Apologies if Peter and I weren't clear.

[mlamouri (slow - plz ping), 2014-03-24 17:46:52]

On 2014/03/21 18:10:57, Dirk Pranke wrote:
> Yes. Apologies if Peter and I weren't clear.

No worry. I've updated the patch. PTAL.

[Dirk Pranke, 2014-03-24 18:14:59]

lgtm w/ one nit ... use "_build_requirements_link", not
"_get_build_requirements_link" . In fact, you could probably just make this a
class-level constant and just say self.build_requirements_url or something.

We don't tend to use the "get_/set_" naming conventions in our python code.

(apologies if I suggested the wrong name before).

If you felt like making that change in a separate CL or punting it to me that
would be fine, too ...

[mlamouri (slow - plz ping), 2014-03-25 14:33:52]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-25 14:33:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/196413027/80001

[mlamouri (slow - plz ping), 2014-03-25 15:11:23]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-03-25 15:12:01]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-25 15:12:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/196413027/80001

[commit-bot: I haz the power, 2014-03-25 15:40:21]

Change committed as 169952