Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
179173003:
Refactor compiler and runtime configurations (command line tools command building). (Closed)
|
Created:
6 years, 9 months ago by ahe Modified:
6 years, 9 months ago CC:
reviews_dartlang.org, ricow1 Visibility:
Public. |
DescriptionRefactor compiler and runtime configurations (command line tools command building).
R=kustermann@google.com
Committed: https://code.google.com/p/dart/source/detail?r=33197
Commit included https://codereview.chromium.org/141713002/
Patch Set 1 : #
Total comments: 48
Patch Set 2 : Address comments, sorta. #
Total comments: 11
Messages
Total messages: 20 (0 generated)
First round of comments. In general: We've these "new CompilerConfiguration()" calls all over in test_suite.dart even though the configuration doesn't change for a given TestSuite. Why not computing this object in test_options, when the "Map configuration" is also created? We also have these 'TestUtils.ensureExists()' calls all over. There is no need to do this again and again. Can't we do part of this validation in test_options? I mean we create configurations, and then we can test if "-cdart2js -rff --use-sdk" configuration makes sense. i.e. we can validate at that point, that the SDK was built (since the options say --use-sdk). ]It's also a huge amount of "boilerplate looking" code that comes with all of this, the CompilerConfiguration/RuntimeConfiguration are now 500+.] https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:116: } I'm not sure I like this. A [CompilerConfiguration] class should represent the configuration (e.g. options like csp, ...), should be able to answer questions about the configuration (what options do you have, what would be a sensible timeout). It's a global thing and it's the same thing for all tests in a TestSuite. But, I'm not sure if it should build Command objects. Comments: - The name 'computeCompilationArtifact' is misleading: it's not computing the artifact, it's rather building the commands to build the artifact. Maybe we could rename this to 'buildCompilationCommands()', because that's really what it's doing. - it's strange that TestSuite calls 'CompilationConfiguration.computeRuntimeArguments()' and then takes these arguments, and passes them back to 'CompilerConfiguration.computeRuntimeArguments()' again. - I'd argue that CommandArtifact.{filename,mimeType} is something that the Command should know: If I have a CompilationCommand, then that command should know what it's output will be, why not introducing that at the Command class, e.g. class Command { // A list of artifacts that should be available after running this command. List<Artifact> get expectedArtifacts; } class Artifact {} class FileArtifact extends Artifact { final String filename; final String mimeType; } https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:968: tempDir = createCompilationOutputDirectory(info.filePath); This is not correct. 'compilerConfiguration.hasCompiler' is true for the analyzer, but there is really no need to create directories, since the analyzer doesn't output anything (as opposed to dart2js/dart2dart). [It is also pretty strange in general that we consider the analyzer as a compiler even though it's not compiling anything.] https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:1327: dart2JsBootstrapDependencies, compilerPath, args, environmentOverrides); You introduced 500+ lines with CompilerConfiguration/RuntimeConfiguration and here we still don't use them. Why is that?
https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:87: this.isChecked: false, Should these arguments be required, since they are being provided at every use site? Are default values a good idea at all? https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:92: // TODO(ahe): Convert to getter? Sure, getter. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:159: final String moniker; Why not name, or compilerName? Mon ikke this will confuse some people. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:204: if (!useSdk) const <Uri>[]; return const <Uri>[]? https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:322: return 4; Are we sure that this 4 was not multiplied by debug and check factors somewhere else, in the original code? It seems strange. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/runtime_configuration.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:119: commandBuilder.getJSCommandlineCommand( Is the idea to move these functions over into runtime_configuration as part of a later refactoring? https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:169: class DrtRuntimeConfiguration extends DartVmRuntimeConfiguration { Where is computeRuntimeCommands for this class? https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:203: // TODO(ahe): Remove this class. Comment also where the class is used. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:146: // using this getter. True. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:995: return commands very nice. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:1323: executable = dartVmBinaryFileName; We lose the special handling of useSdk when we make this change. Is this correct?
Thank you, Martin. I've replied to your general comments below. I want to make it clear that this is work in progress. I am committed to getting rid of all occurrences of configuration['compiler'] and configuration['runtime'] outside test_options.dart. But I'm also worried about breaking our test infrastructure, and generally like to work in an incremental fashion. This particular CL was cut immediately after I had gotten rid of configuration['compiler'] and configuration['runtime'] from makeCommands. That was really complicated, so I felt this was a good time for a review. On 2014/02/28 09:39:55, kustermann wrote: > We've these "new CompilerConfiguration()" calls all over in test_suite.dart even > though the configuration doesn't change for a given TestSuite. Why not computing > this object in test_options, when the "Map configuration" is also created? Because I think this implies storing the CompilerConfiguration in a map and looking it up by a string. That's annoying to search for, so I'm deliberately using something that is too verbose, but easy to search for. What I would really like to do is add it as a property to the configuration, but since configuration is just a Map, that is another refactoring altogether. > We also have these 'TestUtils.ensureExists()' calls all over. There is no need > to do this again and again. Can't we do part of this validation in test_options? > I mean we create configurations, and then we can test if "-cdart2js -rff > --use-sdk" configuration makes sense. i.e. we can validate at that point, that > the SDK was built (since the options say --use-sdk). I think that is a good idea. Right now, I'm focusing on creating objects that represent compilers and runtimes, and I'm trying to avoid following all possible clean-up opportunities, as I could just go on forever. I've added a TODO, and plan to get to it soon. > ]It's also a huge amount of "boilerplate looking" code that comes with all of > this, the CompilerConfiguration/RuntimeConfiguration are now 500+.] Yes. I don't feel I'm done as long as most of the commands take so many arguments. So I'll be iterating on that.
lgtm with comments I suspect there could be issues when submitting the these two CLs. Looking forward for eliminating the remaining compilation command creation in enqueueBrowserTests(), .... https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:82: } I may have made a comment at the first CL about this. I don't like it if a class knows all of it's subclasses. But you can keep it if you want. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:98: throw "Unknown compiler for: $runtimeType"; Where is this runtimeType variable defined? https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:101: bool get hasCompiler => true; Maybe this could be abstract? If a subclass forgets to override it, then it could be a hard-to-find bug. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:105: bool get isCsp => false; Isn't this Dart2xCompilerConfiguration specific? The analyzer doesn't care about '--csp' right? https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:171: String computeCompilerPath(String buildDir) { Note for the future: The buildDir is also fixed for a given configuration. It could also be computed *once* (maybe in test_options) and passed in here in the constructor as all the other values. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:299: // TODO(antonm): support checked. Not sure why you're adding TODO's for anton: I think he'll never "DO" it :) https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/runtime_configuration.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:74: } Make it abstract? https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:103: } Could you make a class RuntimeConfiguration { .. assertMimetype(CommandArtifact artifact, String expectedMimetype) { if ( ...) throw ...; } } And call it from all subclasses? https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:119: commandBuilder.getJSCommandlineCommand( On 2014/02/28 09:53:08, Bill Hesse wrote: > Is the idea to move these functions over into runtime_configuration as part of a > later refactoring? These functions perform canonicalization. I don't think Command canonicalization code belongs here. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:138: moniker, suite.jsShellFileName, arguments, environmentOverrides)]; Note for the future: All these path building functions in TestSuite (e.g. jsShellFileName) could also be computed after test_options has expanded configurations (i.e. we could rip it out of TestSuite). https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:139: } Could we eliminate the duplication of JsshellRuntimeConfiguration/D8RuntimeConfiguration.computeRuntimeCommands ? https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:169: class DrtRuntimeConfiguration extends DartVmRuntimeConfiguration { On 2014/02/28 09:53:08, Bill Hesse wrote: > Where is computeRuntimeCommands for this class? See my comment above: it should be abstract in RuntimeConfiguration, to make sure we detect missing overrides.
Thank you, Bill and Martin! The plan is to move this CL and CL 141713002 to tip of tree and then submit this CL in the morning after a "green" night. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:82: } On 2014/02/28 11:06:29, kustermann wrote: > I may have made a comment at the first CL about this. I don't like it if a class > knows all of it's subclasses. But you can keep it if you want. I'm not sure I want to keep the factory approach, and when I replace it, I'll consider this point. I'm not sure I agree with your principle, but as I'm leaning towards getting rid of the factory, I think we agree on this particular example. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:87: this.isChecked: false, On 2014/02/28 09:53:08, Bill Hesse wrote: > Should these arguments be required, since they are being provided at every use > site? Are default values a good idea at all? We discussed a different approach that I'll try out. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:92: // TODO(ahe): Convert to getter? On 2014/02/28 09:53:08, Bill Hesse wrote: > Sure, getter. OK, changed to an exclamation point :-) https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:98: throw "Unknown compiler for: $runtimeType"; On 2014/02/28 11:06:29, kustermann wrote: > Where is this runtimeType variable defined? In Object. It returns an instance of Type whose toString gives you the name of the class. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:101: bool get hasCompiler => true; On 2014/02/28 11:06:29, kustermann wrote: > Maybe this could be abstract? If a subclass forgets to override it, then it > could be a hard-to-find bug. Not sure. I think all subclasses has a compiler except "none". https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:105: bool get isCsp => false; On 2014/02/28 11:06:29, kustermann wrote: > Isn't this Dart2xCompilerConfiguration specific? > The analyzer doesn't care about '--csp' right? I can probably remove this. Adding TODO. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:116: } On 2014/02/28 09:39:55, kustermann wrote: > I'm not sure I like this. > > A [CompilerConfiguration] class should represent the configuration (e.g. options > like csp, ...), should be able to answer questions about the configuration (what > options do you have, what would be a sensible timeout). It's a global thing and > it's the same thing for all tests in a TestSuite. > > But, I'm not sure if it should build Command objects. Somebody has to build command objects, and building command objects depend heavily on the compiler. So to me, building command objects for invoking the compiler is an integral part of the compiler configuration object. > Comments: > > - The name 'computeCompilationArtifact' is misleading: it's not computing the > artifact, it's rather building the commands to build the artifact. Maybe we > could rename this to 'buildCompilationCommands()', because that's really what > it's doing. I think the issue is that CommandArtifact isn't a great name for that class. The intent of the class is to abstract over something that is built (including how it is built). But I'm having a really hard time finding a great name for this concept. > - it's strange that TestSuite calls > 'CompilationConfiguration.computeRuntimeArguments()' and then takes these > arguments, and passes them back to > 'CompilerConfiguration.computeRuntimeArguments()' again. I think you meant that they are passed back to runtimeConfiguration.computeRuntimeCommands. I'm not sure if I agree. The alternative I see is passing the compiler configuration to runtimeConfiguration.computeRuntimeCommands. The compiler and runtime need to coordinate (in the dart2dart case). > - I'd argue that CommandArtifact.{filename,mimeType} is something that the > Command should know: If I have a CompilationCommand, then that command should > know what it's output will be, why not introducing that at the Command class, > e.g. > class Command { > // A list of artifacts that should be available after running this command. > List<Artifact> get expectedArtifacts; > } > > class Artifact {} > > class FileArtifact extends Artifact { > final String filename; > final String mimeType; > } I agree this is nice, but I don't think it would work. A command can produce several artifacts, but it cannot have a favorite as long as we want the commands to get canonicalized across different configurations. For example, invoking dart2js once produces both out.js and out.precompiled.js, and the CommandArtifact encapsulates which of them you want (and how to build it). https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:159: final String moniker; On 2014/02/28 09:53:08, Bill Hesse wrote: > Why not name, or compilerName? Mon ikke this will confuse some people. 1. I don't want this to be a general thing. I don't just want to make a field that replaces configuration['compiler']. 2. The compiler is always named dart2js, there is no compiler named dart2dart. It is a nickname, and according to Merriam-Webster, a that is what moniker means. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:171: String computeCompilerPath(String buildDir) { On 2014/02/28 11:06:29, kustermann wrote: > Note for the future: > The buildDir is also fixed for a given configuration. It could also be computed > *once* (maybe in test_options) and passed in here in the constructor as all the > other values. > Good point. Added TODO. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:204: if (!useSdk) const <Uri>[]; On 2014/02/28 09:53:08, Bill Hesse wrote: > return const <Uri>[]? Nice catch! https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:299: // TODO(antonm): support checked. On 2014/02/28 11:06:29, kustermann wrote: > Not sure why you're adding TODO's for anton: I think he'll never "DO" it :) I know. I copied it. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:322: return 4; On 2014/02/28 09:53:08, Bill Hesse wrote: > Are we sure that this 4 was not multiplied by debug and check factors somewhere > else, in the original code? It seems strange. I'm sure. See https://codereview.chromium.org/141713002/diff/80001/dart/tools/testing/dart/... https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/runtime_configuration.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:74: } On 2014/02/28 11:06:29, kustermann wrote: > Make it abstract? Added TODO. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:103: } On 2014/02/28 11:06:29, kustermann wrote: > Could you make a > class RuntimeConfiguration { > .. > assertMimetype(CommandArtifact artifact, String expectedMimetype) { > if ( ...) throw ...; > } > } > > And call it from all subclasses? So far, they are slightly different due to the fact that there i no artifact for the VM for normal VM tests. Not yet sure how to handle this elegantly. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:119: commandBuilder.getJSCommandlineCommand( On 2014/02/28 11:06:29, kustermann wrote: > On 2014/02/28 09:53:08, Bill Hesse wrote: > > Is the idea to move these functions over into runtime_configuration as part of > a > > later refactoring? > > These functions perform canonicalization. I don't think Command canonicalization > code belongs here. I'm not sure. I would really like to have everything about a compiler in one place, but Martin makes a good point. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:138: moniker, suite.jsShellFileName, arguments, environmentOverrides)]; On 2014/02/28 11:06:29, kustermann wrote: > Note for the future: > All these path building functions in TestSuite (e.g. jsShellFileName) could also > be computed after test_options has expanded configurations (i.e. we could rip it > out of TestSuite). I've made a note/TODO about this. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:139: } On 2014/02/28 11:06:29, kustermann wrote: > Could we eliminate the duplication of > JsshellRuntimeConfiguration/D8RuntimeConfiguration.computeRuntimeCommands ? I'm not really sure. I hope this method can be simplified to take less arguments. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:169: class DrtRuntimeConfiguration extends DartVmRuntimeConfiguration { On 2014/02/28 11:06:29, kustermann wrote: > On 2014/02/28 09:53:08, Bill Hesse wrote: > > Where is computeRuntimeCommands for this class? > > See my comment above: it should be abstract in RuntimeConfiguration, to make > sure we detect missing overrides. Right now, it isn't used for "browser" runtimes. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/runtime_configuration.dart:203: // TODO(ahe): Remove this class. On 2014/02/28 09:53:08, Bill Hesse wrote: > Comment also where the class is used. Done. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... File dart/tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:968: tempDir = createCompilationOutputDirectory(info.filePath); On 2014/02/28 09:39:55, kustermann wrote: > This is not correct. 'compilerConfiguration.hasCompiler' is true for the > analyzer, but there is really no need to create directories, since the analyzer > doesn't output anything (as opposed to dart2js/dart2dart). > > [It is also pretty strange in general that we consider the analyzer as a > compiler even though it's not compiling anything.] I've added a TODO. I think the solution is to move into the compiler configuration. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:1323: executable = dartVmBinaryFileName; On 2014/02/28 09:53:08, Bill Hesse wrote: > We lose the special handling of useSdk when we make this change. Is this > correct? I'm not sure I understand. dartVmBinaryFileName checks for useSdk. https://codereview.chromium.org/179173003/diff/20001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:1327: dart2JsBootstrapDependencies, compilerPath, args, environmentOverrides); On 2014/02/28 09:39:55, kustermann wrote: > You introduced 500+ lines with CompilerConfiguration/RuntimeConfiguration and > here we still don't use them. Why is that? As discussed in person, I'm not done yet.
Message was sent while issue was closed.
Committed patchset #2 manually as r33197 (presubmit successful).
Message was sent while issue was closed.
Some late comments, sorry about that https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... File dart/tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:26: final String filename; I am not sure expected result is a good comment here and maybe even filename should be more descriptive, like outputFilename (when I first read this I immediately thought that this was the expected output of running the command (as in stdout) https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:54: // TODO(ahe): Move these booleans into a struction configuration object +1 on that https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/compiler_configuration.dart:166: final String moniker; could this just be toolName - I had to look up moniker. https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... File dart/tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:132: // TODO(rnystrom): Eventually, all test suites should run out of the SDK where and why was this introduced - as stated below this seems very broken https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:964: String tempDir; wouldn't a better name be outputDir here? https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:997: return commands I would not do a method cascade here, but call the method on commands above instead, this seems to complicate (as in decreased readability) more than it beautifies - but that could just be me,
Message was sent while issue was closed.
Thank you, Rico. I'll iterate on this in the coming days and address your comments. Cheers, Peter On 2014/03/03 07:47:08, ricow1 wrote: > Some late comments, sorry about that > > https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... > File dart/tools/testing/dart/compiler_configuration.dart (right): > > https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... > dart/tools/testing/dart/compiler_configuration.dart:26: final String filename; > I am not sure expected result is a good comment here and maybe even filename > should be more descriptive, like outputFilename (when I first read this I > immediately thought that this was the expected output of running the command (as > in stdout) > > https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... > dart/tools/testing/dart/compiler_configuration.dart:54: // TODO(ahe): Move these > booleans into a struction configuration object > +1 on that > > https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... > dart/tools/testing/dart/compiler_configuration.dart:166: final String moniker; > could this just be toolName - I had to look up moniker. > > https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... > File dart/tools/testing/dart/test_suite.dart (right): > > https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... > dart/tools/testing/dart/test_suite.dart:132: // TODO(rnystrom): Eventually, all > test suites should run out of the SDK > where and why was this introduced - as stated below this seems very broken > > https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... > dart/tools/testing/dart/test_suite.dart:964: String tempDir; > wouldn't a better name be outputDir here? > > https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... > dart/tools/testing/dart/test_suite.dart:997: return commands > I would not do a method cascade here, but call the method on commands above > instead, this seems to complicate (as in decreased readability) more than it > beautifies - but that could just be me,
Message was sent while issue was closed.
https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... File dart/tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:132: // TODO(rnystrom): Eventually, all test suites should run out of the SDK On 2014/03/03 07:47:08, ricow1 wrote: > where and why was this introduced - as stated below this seems very broken This was added a long time ago when I first started adding support for testing code that contained "package:" imports. I don't remember much about it, so feel free to delete or change it as you or Peter see fit. https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:140: // developers. Agreed. I'm not sure what I meant by that comment. https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:141: // Third, even if pub can only run from the SDK directory, this is the The pub binary runs fine from <repo>/sdk/bin/pub, and it will correctly run the latest pub source code directly from the repo. However, it does need the SDK to have been built because it uses the dart VM executable from the built SDK. Pub can't run using xcodebuild/ReleaseIA32/dart (or whatever) because it needs access to the "version" file that gets built in the SDK.
Message was sent while issue was closed.
https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... File dart/tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:132: // TODO(rnystrom): Eventually, all test suites should run out of the SDK On 2014/03/03 21:45:51, Bob Nystrom wrote: > On 2014/03/03 07:47:08, ricow1 wrote: > > where and why was this introduced - as stated below this seems very broken > > This was added a long time ago when I first started adding support for testing > code that contained "package:" imports. I don't remember much about it, so feel > free to delete or change it as you or Peter see fit. Thank you, Bob. It's awesome that you noticed this. I should have cc'ed you, so you could respond directly. https://codereview.chromium.org/179173003/diff/40001/dart/tools/testing/dart/... dart/tools/testing/dart/test_suite.dart:141: // Third, even if pub can only run from the SDK directory, this is the On 2014/03/03 21:45:51, Bob Nystrom wrote: > The pub binary runs fine from <repo>/sdk/bin/pub, and it will correctly run the > latest pub source code directly from the repo. > > However, it does need the SDK to have been built because it uses the dart VM > executable from the built SDK. Pub can't run using xcodebuild/ReleaseIA32/dart > (or whatever) because it needs access to the "version" file that gets built in > the SDK. OK. So I wonder if we should fix this. It would seem to me that if we built a version file along with .../ReleaseIA32/dart, pub could read that.
Message was sent while issue was closed.
On 2014/03/04 07:46:38, ahe wrote: > Thank you, Bob. It's awesome that you noticed this. I should have cc'ed you, so > you could respond directly. You can give Rietveld credit for this. The code review showed up in my inbox, I guess because my name was mentioned? <shrug> > OK. So I wonder if we should fix this. It would seem to me that if we built a > version file along with .../ReleaseIA32/dart, pub could read that. In the SDK, the version file is up one directory from the Dart executable, so we'd have to tweak pub to take that into account. If you think it will make your life easier to support that, I'm fine with it. Cheers! - bob
Message was sent while issue was closed.
On 2014/03/04 16:56:15, Bob Nystrom wrote: > If you think it will make your life easier to support that, I'm fine with it. I'm doing some refactorings that should make it easy to move the special case to test_options.dart. It doesn't matter much to me whether or not pub can run from sdk/bin, I only offered to modify the build system in case it would help pub development.
Message was sent while issue was closed.
On 2014/03/05 11:19:21, ahe wrote: > On 2014/03/04 16:56:15, Bob Nystrom wrote: > It doesn't matter much to me whether or not pub can run from sdk/bin, I only > offered to modify the build system in case it would help pub development. I definitely appreciate the offer, but I think our workflow is fine now. We basically build the SDK once on sync and then can iterate directly on the pub source after that. Cheers! - bob
Message was sent while issue was closed.
On 2014/03/05 19:36:30, Bob Nystrom wrote: > On 2014/03/05 11:19:21, ahe wrote: > > On 2014/03/04 16:56:15, Bob Nystrom wrote: > > It doesn't matter much to me whether or not pub can run from sdk/bin, I only > > offered to modify the build system in case it would help pub development. > > I definitely appreciate the offer, but I think our workflow is fine now. We > basically build the SDK once on sync and then can iterate directly on the pub > source after that. > > Cheers! > > - bob I'm curious. How do you do that? Can you run pub from sdk/bin after building the SDK?
Message was sent while issue was closed.
On 2014/03/05 19:54:58, ahe wrote: > On 2014/03/05 19:36:30, Bob Nystrom wrote: > > On 2014/03/05 11:19:21, ahe wrote: > > > On 2014/03/04 16:56:15, Bob Nystrom wrote: > > > It doesn't matter much to me whether or not pub can run from sdk/bin, I only > > > offered to modify the build system in case it would help pub development. > > > > I definitely appreciate the offer, but I think our workflow is fine now. We > > basically build the SDK once on sync and then can iterate directly on the pub > > source after that. > > > > Cheers! > > > > - bob > > I'm curious. How do you do that? Can you run pub from sdk/bin after building the > SDK? Yup. The shell script does something similar to dart2js where it detects if it's running from a snapshot or not. If not, it uses the Dart executable from the built SDK directory, but it runs the pub.dart entrypoint directly from the repo. Since pub locates the version file relative to the Dart executable and not the entrypoint, it's still able to find the version file. - bob
Message was sent while issue was closed.
On 2014/03/05 21:06:05, Bob Nystrom wrote: > On 2014/03/05 19:54:58, ahe wrote: > > On 2014/03/05 19:36:30, Bob Nystrom wrote: > > > On 2014/03/05 11:19:21, ahe wrote: > > > > On 2014/03/04 16:56:15, Bob Nystrom wrote: > > > > It doesn't matter much to me whether or not pub can run from sdk/bin, I > only > > > > offered to modify the build system in case it would help pub development. > > > > > > I definitely appreciate the offer, but I think our workflow is fine now. We > > > basically build the SDK once on sync and then can iterate directly on the > pub > > > source after that. > > > > > > Cheers! > > > > > > - bob > > > > I'm curious. How do you do that? Can you run pub from sdk/bin after building > the > > SDK? > > Yup. > > The shell script does something similar to dart2js where it detects if it's > running from a snapshot or not. If not, it uses the Dart executable from the > built SDK directory, but it runs the pub.dart entrypoint directly from the repo. > > Since pub locates the version file relative to the Dart executable and not the > entrypoint, it's still able to find the version file. > > - bob Actually Peter and I had already a discussion about all of this (where I also explained how it works -- I've investigated this back then when I added the pkgbuild testsuite). @Bob: You might think it's not important to be able to run pub outside of the SDK, but IMHO it is important. For several reasons, most importantly: a) All our vm builders on the buildbot build only the "runtime" target: This means pub cannot run on these builders. Which means test.py cannot create a real package-root for sample/pkg/... tests using pub on these builders (i.e. respecting what pubspec.yaml says, not using the out/ReleaseIA32/packages hack)! Depending on in which direction we want to go with our sample/pkg testing, we may need to run pub there. b) Our mips/arm builders also build only the "runtime" target. They cannot build the SDK, because java might not be available on our mips/arm HW (we might be able to get java on these, but let's put that aside) -- this will go away as soon as the java-based analyzer is completely replaced by the dart-based one. But the status quo is: Pub is therefore not running on our arm/mips builders. Pub has been a great tool to discover bugs (in particular in dart:io): Now that test.py will work closer with pub to create package-roots, run 'pub build', ... I'd strongly prefer if we can get that working on our mips/arm builders as well.
Message was sent while issue was closed.
On 2014/03/05 23:28:59, kustermann wrote: > Actually Peter and I had already a discussion about all of this (where I also > explained how it works -- I've investigated this back then when I added the > pkgbuild testsuite). I didn't understand it before :-) > @Bob: > You might think it's not important to be able to run pub outside of the SDK, but > IMHO it is important. Bob said that if made our life easier, he'd be all for changing pub. I don't think that's necessary, we just need to simplify the pub script and build a version file as part of runtime. Let's do that. Cheers, Peter
Message was sent while issue was closed.
On 2014/03/05 23:28:59, kustermann wrote: > @Bob: > You might think it's not important to be able to run pub outside of the SDK, but > IMHO it is important. For several reasons, most importantly: I just meant that it's not important *to me*. I don't personally have use cases that require that. I'm totally supportive if others on the team do. :) > a) All our vm builders on the buildbot build only the "runtime" target: This > means pub cannot run on these builders. Which means test.py cannot create a real > package-root for sample/pkg/... tests using pub on these builders (i.e. > respecting what pubspec.yaml says, not using the out/ReleaseIA32/packages hack)! > Depending on in which direction we want to go with our sample/pkg testing, we > may need to run pub there. > > b) Our mips/arm builders also build only the "runtime" target. They cannot > build the SDK, because java might not be available on our mips/arm HW (we might > be able to get java on these, but let's put that aside) -- this will go away as > soon as the java-based analyzer is completely replaced by the dart-based one. > But the status quo is: Pub is therefore not running on our arm/mips builders. > Pub has been a great tool to discover bugs (in particular in dart:io): Now that > test.py will work closer with pub to create package-roots, run 'pub build', ... > I'd strongly prefer if we can get that working on our mips/arm builders as well. +1. - bob
Message was sent while issue was closed.
On 2014/03/06 17:24:07, Bob Nystrom wrote: > On 2014/03/05 23:28:59, kustermann wrote: > > @Bob: > > You might think it's not important to be able to run pub outside of the SDK, > but > > IMHO it is important. For several reasons, most importantly: > > I just meant that it's not important *to me*. I don't personally have use cases > that require that. I'm totally supportive if others on the team do. :) > > > a) All our vm builders on the buildbot build only the "runtime" target: This > > means pub cannot run on these builders. Which means test.py cannot create a > real > > package-root for sample/pkg/... tests using pub on these builders (i.e. > > respecting what pubspec.yaml says, not using the out/ReleaseIA32/packages > hack)! > > Depending on in which direction we want to go with our sample/pkg testing, we > > may need to run pub there. > > > > b) Our mips/arm builders also build only the "runtime" target. They cannot > > build the SDK, because java might not be available on our mips/arm HW (we > might > > be able to get java on these, but let's put that aside) -- this will go away > as > > soon as the java-based analyzer is completely replaced by the dart-based one. > > But the status quo is: Pub is therefore not running on our arm/mips builders. > > Pub has been a great tool to discover bugs (in particular in dart:io): Now > that > > test.py will work closer with pub to create package-roots, run 'pub build', > ... > > I'd strongly prefer if we can get that working on our mips/arm builders as > well. > > +1. > > - bob Sounds like we all agree :-) Cheers, Peter |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
