Bug 54533 – new-run-webkit-tests should find DumpRenderTree even if 'ninja' is used as a build system.

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Hayato Ito]

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.

[Hayato Ito]

Created attachment 82595 [details]
make-nrwt-ninja-friendly

[Kent Tamura]

Comment on attachment 82595 [details]
make-nrwt-ninja-friendly

Can you write a test for new behavior?

[Hayato Ito]

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?

[Hayato Ito]

Created attachment 82599 [details]
add-test

[Hayato Ito]

I've added a test.

[Hayato Ito]

Created attachment 82600 [details]
rename-variables

[Dirk Pranke]

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.

[Hayato Ito]

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?

[Tony Chang]

(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.

[Evan Martin]

I agree with Tony.  I'd prefer to not to make WebKit more complex for this.

[Dirk Pranke]

(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?

[Tony Chang]

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.

[Hayato Ito]

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.

[Tony Chang]

*** Bug 56501 has been marked as a duplicate of this bug. ***

[James Robinson]

I'm using symlinks as well and am pretty happy with that and a few aliases.

[Eric Seidel (no email)]

This sounds easy to solve.  Is this still an issue?

[Hayato Ito]

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?

[Tony Chang]

Marking the bug as wontfix for now.  If ninja usage becomes an officially supported tool, we can reopen this bug.