Bug 54533 - new-run-webkit-tests should find DumpRenderTree even if 'ninja' is used as a build system.
Summary: new-run-webkit-tests should find DumpRenderTree even if 'ninja' is used as a ...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 56501 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-02-15 23:34 PST by Hayato Ito
Modified: 2011-08-09 09:38 PDT (History)
6 users (show)

See Also:


Attachments
make-nrwt-ninja-friendly (1.81 KB, patch)
2011-02-15 23:41 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
add-test (4.20 KB, patch)
2011-02-16 00:50 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
rename-variables (4.32 KB, patch)
2011-02-16 00:55 PST, Hayato Ito
tony: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2011-02-15 23:34:25 PST
Ninja, new build system is now available.
http://neugierig.org/software/chromium/notes/2011/02/ninja.html

A new-run-webkit-test should find DumpRenderTree even if ninja is used as a build system.
Comment 1 Hayato Ito 2011-02-15 23:41:17 PST
Created attachment 82595 [details]
make-nrwt-ninja-friendly
Comment 2 Kent Tamura 2011-02-15 23:43:36 PST
Comment on attachment 82595 [details]
make-nrwt-ninja-friendly

Can you write a test for new behavior?
Comment 3 Hayato Ito 2011-02-16 00:04:38 PST
Let me try. It seems that we have to manipulate mock_filesystem to write a test.

(In reply to comment #2)
> (From update of attachment 82595 [details])
> Can you write a test for new behavior?
Comment 4 Hayato Ito 2011-02-16 00:50:45 PST
Created attachment 82599 [details]
add-test
Comment 5 Hayato Ito 2011-02-16 00:51:26 PST
I've added a test.
Comment 6 Hayato Ito 2011-02-16 00:55:44 PST
Created attachment 82600 [details]
rename-variables
Comment 7 Dirk Pranke 2011-02-16 14:23:34 PST
Comment on attachment 82600 [details]
rename-variables

View in context: https://bugs.webkit.org/attachment.cgi?id=82600&action=review

adding evan to this. Evan, is ninja intentionally ignoring the configuration? That doesn't seem like a good thing.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:94
> +                    if arg.lower() not in ['release', 'debug', 'default']]

This is kind of an ugly hack. If it is correct that ninja ignores the configuration, then I think it would be better to change the signature of _build_path() to be def _build_path(configuration, binary) and then just ignore the configuration parameter here. 

However, I'm not real comfortable with ninja ignoring the configuration. That seems like it could cause bad things could happen and is unlike all of our other builds.
Comment 8 Hayato Ito 2011-02-16 19:06:54 PST
Thank you for the review.

(In reply to comment #7)
> (From update of attachment 82600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82600&action=review
> 
> adding evan to this. Evan, is ninja intentionally ignoring the configuration? That doesn't seem like a good thing.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:94
> > +                    if arg.lower() not in ['release', 'debug', 'default']]
> 
> This is kind of an ugly hack. If it is correct that ninja ignores the configuration, then I think it would be better to change the signature of _build_path() to be def _build_path(configuration, binary) and then just ignore the configuration parameter here. 
> 
> However, I'm not real comfortable with ninja ignoring the configuration. That seems like it could cause bad things could happen and is unlike all of our other builds.

I agree that this is an ugly hack. I've taken look at code of ninja (tools/gyp/pylib/gyp/generator/ninja.py), and it doesn't seem to be difficult to let ninja use different directory (say, ninja/Debug or ninja/Release) according to given configuration, looking at "-Gconfig" option.

Evan, what do you think about this?
Comment 9 Tony Chang 2011-02-22 09:47:21 PST
(In reply to comment #7)
> (From update of attachment 82600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82600&action=review
> 
> adding evan to this. Evan, is ninja intentionally ignoring the configuration? That doesn't seem like a good thing.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:94
> > +                    if arg.lower() not in ['release', 'debug', 'default']]
> 
> This is kind of an ugly hack. If it is correct that ninja ignores the configuration, then I think it would be better to change the signature of _build_path() to be def _build_path(configuration, binary) and then just ignore the configuration parameter here. 
> 
> However, I'm not real comfortable with ninja ignoring the configuration. That seems like it could cause bad things could happen and is unlike all of our other builds.

In ninja, the configuration is part of the gclient step.

I feel that whoever is using ninja right now should just symlink out/{Debug,Release} to ninja/.  It's not quite ready for prime time yet.
Comment 10 Evan Martin 2011-02-22 11:52:41 PST
I agree with Tony.  I'd prefer to not to make WebKit more complex for this.
Comment 11 Dirk Pranke 2011-02-22 11:56:08 PST
(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 82600 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=82600&action=review
> > 
> > adding evan to this. Evan, is ninja intentionally ignoring the configuration? That doesn't seem like a good thing.
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:94
> > > +                    if arg.lower() not in ['release', 'debug', 'default']]
> > 
> > This is kind of an ugly hack. If it is correct that ninja ignores the configuration, then I think it would be better to change the signature of _build_path() to be def _build_path(configuration, binary) and then just ignore the configuration parameter here. 
> > 
> > However, I'm not real comfortable with ninja ignoring the configuration. That seems like it could cause bad things could happen and is unlike all of our other builds.
> 
> In ninja, the configuration is part of the gclient step.
> 
> I feel that whoever is using ninja right now should just symlink out/{Debug,Release} to ninja/.  It's not quite ready for prime time yet.

What does it mean to say that the configuration is part of the gclient step? The configuration is being read only when the buildfile is generated by GYP?

It looks like you can have variables in .ninja files; is there an obvious downside to just having a $BUILDTYPE get generated by GYP instead?
Comment 12 Tony Chang 2011-02-22 12:45:12 PST
Comment on attachment 82600 [details]
rename-variables

We should reconsider if we decide to switch the default Linux build from make to ninja.  This is similar to what we did when we switched from scons to make.
Comment 13 Hayato Ito 2011-02-22 19:15:54 PST
I am aware that we can use symlink out/{Debug, Release} to ninja. I am actually doing it and fully satisfied with 'symbolic link' solution. 
But it takes some time for me to find that solution. I just wanted other developers not to spend some time on finding such a solution. It is nice to use ninja out of box without such a tweaking.

I don't have a strong opinion that we should support ninja in new-run-webkit-test at this point. So review- is okay for me now until ninja becomes more popular and ready for prime time.

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 82600 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=82600&action=review
> > 
> > adding evan to this. Evan, is ninja intentionally ignoring the configuration? That doesn't seem like a good thing.
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:94
> > > +                    if arg.lower() not in ['release', 'debug', 'default']]
> > 
> > This is kind of an ugly hack. If it is correct that ninja ignores the configuration, then I think it would be better to change the signature of _build_path() to be def _build_path(configuration, binary) and then just ignore the configuration parameter here. 
> > 
> > However, I'm not real comfortable with ninja ignoring the configuration. That seems like it could cause bad things could happen and is unlike all of our other builds.
> 
> In ninja, the configuration is part of the gclient step.
> 
> I feel that whoever is using ninja right now should just symlink out/{Debug,Release} to ninja/.  It's not quite ready for prime time yet.
Comment 14 Tony Chang 2011-03-16 16:37:49 PDT
*** Bug 56501 has been marked as a duplicate of this bug. ***
Comment 15 James Robinson 2011-03-18 18:40:18 PDT
I'm using symlinks as well and am pretty happy with that and a few aliases.
Comment 16 Eric Seidel (no email) 2011-08-08 10:11:41 PDT
This sounds easy to solve.  Is this still an issue?
Comment 17 Hayato Ito 2011-08-08 18:41:47 PDT
I don't think it is difficult to solve this issue.

But we were not sure whether we should make new-run-webkit-tests aware of Ninja because Ninja was not widely used when this bug was filed.

One more concern is that Ninja itself should change its output directory according to 'debug' build or 'release' build to avoid ugly hack in the patch.

I guess everyone is happy with symbolic link solution. So no one doesn't pay attention to this issue.
But forcing making symbolic links is not friendly to new comers. So I think it is worth solving the issue.

(In reply to comment #16)
> This sounds easy to solve.  Is this still an issue?
Comment 18 Tony Chang 2011-08-09 09:38:43 PDT
Marking the bug as wontfix for now.  If ninja usage becomes an officially supported tool, we can reopen this bug.