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.
Created attachment 82595 [details] make-nrwt-ninja-friendly
Comment on attachment 82595 [details] make-nrwt-ninja-friendly Can you write a test for new behavior?
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?
Created attachment 82599 [details] add-test
I've added a test.
Created attachment 82600 [details] rename-variables
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.
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?
(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.
I agree with Tony. I'd prefer to not to make WebKit more complex for this.
(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 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.
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.
*** Bug 56501 has been marked as a duplicate of this bug. ***
I'm using symlinks as well and am pretty happy with that and a few aliases.
This sounds easy to solve. Is this still an issue?
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?
Marking the bug as wontfix for now. If ninja usage becomes an officially supported tool, we can reopen this bug.