Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
628363002:
Modify dartanalyzer scripts to look more like dart2js scripts. (Closed)
|
Created:
6 years, 2 months ago by Paul Berry Modified:
6 years, 2 months ago CC:
reviews_dartlang.org, ricow1, srdjan, Johnni Winther Visibility:
Public. |
DescriptionModify dartanalyzer scripts to look more like dart2js scripts.
The dartanalyzer script (and its Windows companion, dartanalyzer.bat)
now support:
- Executing from snapshot if present, otherwise from source (needed
for buildbot tests).
- Passing in the appropriate package root when executind from source.
- Passing in VM tuning parameters to speed up analysis.
- Passing through extra DART_VM_OPTIONS to the VM.
Also, the old editor/tools/analyzer script has been removed and
the test infrastructure now uses sdk/bin/dartanalyzer, in the
same way that it works for dart2js.
R=ahe@google.com, johnniwinther@google.com, ricow@google.com
Committed: https://code.google.com/p/dart/source/detail?r=41150
Patch Set 1 #Patch Set 2 : Fix handling of package root. #Patch Set 3 : Remove editor/tools/analyzer. #
Total comments: 7
Patch Set 4 : Address some code review comments. #Patch Set 5 : Move code inside else clause #
Total comments: 1
Patch Set 6 : Rework based on code review comments #
Total comments: 8
Patch Set 7 : Address code review comments and rebase #
Total comments: 2
Messages
Total messages: 29 (3 generated)
paulberry@google.com changed reviewers: + ahe@google.com, ricow@google.com
Rico-- I still haven't had a chance to set up a Windows dev image. Would you mind double checking to make sure my .bat changes are correct? Thanks!
Removing editor/tools/analyzer LGTM
https://codereview.chromium.org/628363002/diff/40001/sdk/bin/dartanalyzer File sdk/bin/dartanalyzer (right): https://codereview.chromium.org/628363002/diff/40001/sdk/bin/dartanalyzer#new... sdk/bin/dartanalyzer:24: DART_ROOT="$(cd "${SDK_DIR}/.." ; pwd -P)" this is not true if we are running from the snapshot https://codereview.chromium.org/628363002/diff/40001/sdk/bin/dartanalyzer#new... sdk/bin/dartanalyzer:32: DART_CONFIGURATION="ReleaseIA32" I really don't like this, and I don't think we need it. We can easily pass the package root from the testing scripts like we do from dart2js in which case we don't need this (and the BUILD_DIR below) https://codereview.chromium.org/628363002/diff/40001/sdk/bin/dartanalyzer#new... sdk/bin/dartanalyzer:39: if [[ `uname` == 'Darwin' ]]; then see comment above https://codereview.chromium.org/628363002/diff/40001/tools/testing/dart/compi... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/628363002/diff/40001/tools/testing/dart/compi... tools/testing/dart/compiler_configuration.dart:405: // return '$prefix/dartanalyzer_developer$suffix'; No commented out code please Additionally, for when you actually do add the code here, I would throw an error if you run with usesdk and isHostChecked
I've applied the patch locally. Then I diff'ed sdk/bin/dart2js and
sdk/bin/dartanalyzer.
The diff contains a number of unexpected changes. In particular:
* Code for locating the package root. I think this code belongs in the script
sdk/bin/dart, as that script isn't shipped.
* A bunch of VM options that aren't associated with a TODO to remove them. I'd
like Srdjan and Ivan to review those options.
--- sdk/bin/dart2js 2014-03-05 12:44:18.000000000 +0100
+++ sdk/bin/dartanalyzer 2014-10-07 12:58:24.000000000 +0200
@@ -1,8 +1,11 @@
#!/bin/bash
-# Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file
+# Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file
# for details. All rights reserved. Use of this source code is governed by a
# BSD-style license that can be found in the LICENSE file.
+# Run dartanalyzer.dart on the Dart VM. This script assumes the Dart SDK's
+# directory structure.
+
function follow_links() {
file="$1"
while [ -h "$file" ]; do
@@ -17,39 +20,37 @@
# Handle the case where dart-sdk/bin has been symlinked to.
BIN_DIR="$(cd "${PROG_NAME%/*}" ; pwd -P)"
-
SDK_DIR="$(cd "${BIN_DIR}/.." ; pwd -P)"
+DART_ROOT="$(cd "${SDK_DIR}/.." ; pwd -P)"
-DART2JS="$SDK_DIR/lib/_internal/compiler/implementation/dart2js.dart"
+ANALYZER="$DART_ROOT/pkg/analyzer/bin/analyzer.dart"
-DART="$BIN_DIR/dart"
+SDK_ARG="--dart-sdk=$SDK_DIR"
-SNAPSHOT_DIR="$BIN_DIR/snapshots"
-SNAPSHOT="$SNAPSHOT_DIR/dart2js.dart.snapshot"
+if [ -z "$DART_CONFIGURATION" ];
+then
+ DART_CONFIGURATION="ReleaseIA32"
+fi
+
+DART="$BIN_DIR/dart"
-unset EXTRA_OPTIONS
-declare -a EXTRA_OPTIONS
+SNAPSHOT="$BIN_DIR/snapshots/dartanalyzer.dart.snapshot"
-if test -t 1; then
- # Stdout is a terminal.
- if test 8 -le `tput colors`; then
- # Stdout has at least 8 colors, so enable colors.
- EXTRA_OPTIONS+=('--enable-diagnostic-colors')
- fi
+if [[ `uname` == 'Darwin' ]]; then
+ BUILD_DIR="$DART_ROOT/xcodebuild/$DART_CONFIGURATION"
+else
+ BUILD_DIR="$DART_ROOT/out/$DART_CONFIGURATION"
fi
+PACKAGE_ROOT="$BUILD_DIR/packages/"
+
unset EXTRA_VM_OPTIONS
declare -a EXTRA_VM_OPTIONS
-if test -f "$SNAPSHOT"; then
- EXTRA_OPTIONS+=("--library-root=$SDK_DIR")
-fi
-
-# Tell the VM to grow the heap more aggressively. This should only
-# be necessary temporarily until the VM is better at detecting how
-# applications use memory.
-# TODO(ahe): Remove this option (http://dartbug.com/6495).
-EXTRA_VM_OPTIONS[${#EXTRA_VM_OPTIONS[@]}]='--heap_growth_rate=512'
+# Set some optimization options that speed up analysis.
+EXTRA_VM_OPTIONS+=("--heap_growth_rate=512")
+EXTRA_VM_OPTIONS+=("--inlining_hotness=90")
+EXTRA_VM_OPTIONS+=("--optimization_counter_threshold=5000")
case $0 in
*_developer)
@@ -64,7 +65,7 @@
fi
if test -f "$SNAPSHOT"; then
- exec "$DART" "${EXTRA_VM_OPTIONS[@]}" "$SNAPSHOT" "${EXTRA_OPTIONS[@]}" "$@"
+ exec "$DART" "${EXTRA_VM_OPTIONS[@]}" "$SNAPSHOT" "$SDK_ARG" "$@"
else
- exec "$DART" "${EXTRA_VM_OPTIONS[@]}" "$DART2JS" "${EXTRA_OPTIONS[@]}" "$@"
+ exec "$DART" "${EXTRA_VM_OPTIONS[@]}" "--package-root=$PACKAGE_ROOT"
"$ANALYZER" "$SDK_ARG" "$@"
fi
https://codereview.chromium.org/628363002/diff/40001/sdk/bin/dartanalyzer File sdk/bin/dartanalyzer (right): https://codereview.chromium.org/628363002/diff/40001/sdk/bin/dartanalyzer#new... sdk/bin/dartanalyzer:24: DART_ROOT="$(cd "${SDK_DIR}/.." ; pwd -P)" On 2014/10/07 06:03:08, ricow1 wrote: > this is not true if we are running from the snapshot Acknowledged. I've moved DART_ROOT and ANALYZER inside the else clause of 'if test -f "$SNAPSHOT"'. https://codereview.chromium.org/628363002/diff/40001/sdk/bin/dartanalyzer#new... sdk/bin/dartanalyzer:32: DART_CONFIGURATION="ReleaseIA32" On 2014/10/07 06:03:08, ricow1 wrote: > I really don't like this, and I don't think we need it. We can easily pass the > package root from the testing scripts like we do from dart2js in which case we > don't need this (and the BUILD_DIR below) I've tried doing this but it's proving to be difficult because the dartanalyzer process is run in batch mode. That means that the only command-line argument seen by this script is the "--batch" argument. So we can't pass the package root to this script using the command line. I also considered passing the package root to this script using an environment variable (e.g. DART_VM_OPTIONS). The problem with that is that if there's a failure, the command printed out by test.py won't be an accurate depiction of the command that was actually run. Anyone who tries to reproduce the failure manually by executing the command will actually be running different code than the code executed by the test script (since they'll be getting the packages from the most recent "pub get" rather than from the package root). So that seems unworkable as well. Suggestions? (Note: for the moment I've moved this if test, and the test for BUILD_DIR below, into the else clause of 'if test -f "$SNAPSHOT"' so that they're out of the picture when run by a normal user from the released SDK. I'm happy to replace them with something better once we figure out what we should do instead.) https://codereview.chromium.org/628363002/diff/40001/tools/testing/dart/compi... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/628363002/diff/40001/tools/testing/dart/compi... tools/testing/dart/compiler_configuration.dart:405: // return '$prefix/dartanalyzer_developer$suffix'; On 2014/10/07 06:03:08, ricow1 wrote: > No commented out code please > > Additionally, for when you actually do add the code here, I would throw an error > if you run with usesdk and isHostChecked Done.
On 2014/10/07 11:07:11, ahe wrote: > I've applied the patch locally. Then I diff'ed sdk/bin/dart2js and > sdk/bin/dartanalyzer. > > The diff contains a number of unexpected changes. In particular: > > * Code for locating the package root. I think this code belongs in the script > sdk/bin/dart, as that script isn't shipped. I can think of three ways this might work: - sdk/bin/dart could pass in the package root all the time. This seems dangerous because developers run sdk/bin/dart manually from time to time, and they will not be expecting it to invisibly set a package root behind the scenes. - sdk/bin/dart could pass in the package root only when it sees a special command-line argument (e.g. "--auto-package-root"). This seems less than ideal, because the special command-line argument will have to come from dartanalyzer (a file which *is* shipped), so we'll be in the confusing situation where a file that *is* shipped makes reference to an undocumented command-line argument that is only handled by code that doesn't ship. In practice it will work fine (since dartanalyzer will only pass the special command-line argument when it can't find the snapshot), but IMHO it seems ugly. - sdk/bin/dart could pass in the package root only when directed to do so by a special environment variable (e.g. AUTO_PACKAGE_ROOT=1). This would have the same disadvantage I noted in my comments to Rico earlier; namely that in the event of a test failure, if the developer tries to manually execute the command line printed out by test.py, they won't be running the same code that the test runner ran. This could lead to a lot of confusion when trying to track down a test failure. Did you have something in mind that I'm missing? None of these alternatives seems terribly good to me, but I'm open to suggestions. > > * A bunch of VM options that aren't associated with a TODO to remove them. I'd > like Srdjan and Ivan to review those options. Ok. For the moment I've added a TODO comment.
iposva@google.com changed reviewers: + iposva@google.com
By using undocumented VM flags your application will break without any warning in the future. -Ivan https://codereview.chromium.org/628363002/diff/60002/sdk/bin/dartanalyzer File sdk/bin/dartanalyzer (right): https://codereview.chromium.org/628363002/diff/60002/sdk/bin/dartanalyzer#new... sdk/bin/dartanalyzer:37: EXTRA_VM_OPTIONS+=("--heap_growth_rate=512") Please do not use any of these flags unless you know exactly what kind of impact they have. Once you have analyzed the impact of these flags, please report and we will work with you to avoid the need of passing them.
On 2014/10/08 04:52:31, Ivan Posva wrote: > By using undocumented VM flags your application will break without any warning > in the future. > > -Ivan > > https://codereview.chromium.org/628363002/diff/60002/sdk/bin/dartanalyzer > File sdk/bin/dartanalyzer (right): > > https://codereview.chromium.org/628363002/diff/60002/sdk/bin/dartanalyzer#new... > sdk/bin/dartanalyzer:37: EXTRA_VM_OPTIONS+=("--heap_growth_rate=512") > Please do not use any of these flags unless you know exactly what kind of impact > they have. Once you have analyzed the impact of these flags, please report and > we will work with you to avoid the need of passing them. Konstantin, can you please follow up with Ivan on these flags? They originated in your commit (r34255). I don't know what they do or whether or not they are still necessary.
On 2014/10/08 04:52:31, Ivan Posva wrote: > By using undocumented VM flags your application will break without any warning > in the future. > > -Ivan > > https://codereview.chromium.org/628363002/diff/60002/sdk/bin/dartanalyzer > File sdk/bin/dartanalyzer (right): > > https://codereview.chromium.org/628363002/diff/60002/sdk/bin/dartanalyzer#new... > sdk/bin/dartanalyzer:37: EXTRA_VM_OPTIONS+=("--heap_growth_rate=512") > Please do not use any of these flags unless you know exactly what kind of impact > they have. Once you have analyzed the impact of these flags, please report and > we will work with you to avoid the need of passing them. Yes, I added these flags a while ago to improve startup time. The time change was pretty significant, both locally and on the performance bot. https://code.google.com/p/dart/source/detail?r=34255 Analyzer is a short-living process, so it should be optimized more aggressively than long-running server processes. Would be nice to have something like Java's --client and --server options in Dart VM.
On 8 October 2013 00:31, ahe wrote: >On Wed Oct 08 2014 at 1:53:58 AM <paulberry@google.com> wrote: >>On 2014/10/07 11:07:11, ahe wrote: >>> I've applied the patch locally. Then I diff'ed sdk/bin/dart2js and >>> sdk/bin/dartanalyzer. >> >>> The diff contains a number of unexpected changes. In particular: >> >>> * Code for locating the package root. I think this code belongs in the >>> script >>> sdk/bin/dart, as that script isn't shipped. >> >>I can think of three ways this might work: >> >>- sdk/bin/dart could pass in the package root all the time. This seems >>dangerous because developers run sdk/bin/dart manually from time to time, >>and >>they will not be expecting it to invisibly set a package root behind the >>scenes. > >That's a great observation. > >I'm not too concerned about this. In this case "developers" refer >to "people who develop the Dart SDK", not "users of the Dart >SDK". > >I actually think this may help Dart SDK developers. When running >tests from the command line using sdk/bin/dart, it would assume >they often would benefit from having the package root set >automatically. > >I've tested, and the VM seems to behave well if you give it >multiple package roots: it uses the last. So if you need to use a >different package root, that should still work with sdk/bin/dart. > >If I'm guessing right, then it seems to me that this option is >superior to the others. You've already done a great job at >shooting them down :-) I'm still concerned. I frequently use sdk/bin/dart to manually execute one-off dart scripts in the course of my day-to-day development. If sdk/dart/bin had started setting the package root without my knowledge, I think I would have been very confused and frustrated. Also, my understanding from talking to Bob Nystrom is that we're trying to move away from using package roots. Modifying the main sdk/bin/dart script to add an implicit package root seems antithetical to that aim. What's your chief concern about adding code to sdk/bin/dartanalyzer that locates the package root? Are you trying to avoid placing this logic in scripts that we ship? If so, how about if I move this logic to a new script (called "sdk/bin/dartanalyzer_local", for example), which the test infrastructure could execute instead of calling dartanalyzer directly? dartanalyzer_local would locate the package root, create the "--package-root=*" flag, and store it in DART_VM_OPTIONS. Then it would execute sdk/bin/dartanalyzer. I think that would address both your concerns and mine.
On 9 October 2014 03:07, ahe wrote: >On Wed Oct 08 2014 at 11:10:05 PM <paulberry@google.com> wrote: >> On 8 October 2013 00:31, ahe wrote: >> > On Wed Oct 08 2014 at 1:53:58 AM <paulberry@google.com> wrote: >> >> On 2014/10/07 11:07:11, ahe wrote: >> >>> I've applied the patch locally. Then I diff'ed sdk/bin/dart2js and >> >>> sdk/bin/dartanalyzer. >> >> >>> The diff contains a number of unexpected changes. In particular: >> >> >>> * Code for locating the package root. I think this code belongs in the >> >>> script >> >>> sdk/bin/dart, as that script isn't shipped. >> >> >> I can think of three ways this might work: >> >> >> - sdk/bin/dart could pass in the package root all the time. This seems >> >> dangerous because developers run sdk/bin/dart manually from time to >> time, >> >> and >> >> they will not be expecting it to invisibly set a package root behind the >> >> scenes. >> >> > That's a great observation. >> >> > I'm not too concerned about this. In this case "developers" refer >> > to "people who develop the Dart SDK", not "users of the Dart >> > SDK". >> >> > I actually think this may help Dart SDK developers. When running >> > tests from the command line using sdk/bin/dart, it would assume >> > they often would benefit from having the package root set >> > automatically. >> >> > I've tested, and the VM seems to behave well if you give it >> > multiple package roots: it uses the last. So if you need to use a >> > different package root, that should still work with sdk/bin/dart. >> >> > If I'm guessing right, then it seems to me that this option is >> > superior to the others. You've already done a great job at >> > shooting them down :-) >> >> I'm still concerned. I frequently use sdk/bin/dart to manually >> execute one-off dart scripts in the course of my day-to-day >> development. If sdk/dart/bin had started setting the package >> root without my knowledge, I think I would have been very >> confused and frustrated. > > >Why? Isn't this simply a matter of implementing it to look for the presence >of a packages directory first? Unfortunately, that won't work, because we develop the analyzer and the analysis server using Dart Editor (for dogfooding purposes), and the Dart Editor automatically runs "pub get", which creates the packages directory. So if we implemented sdk/bin/dart to only set a package root if the packages directory was absent, then when we ran tests locally, we wouldn't be running the same code that the buildbots run. >> Also, my understanding from talking to >> Bob Nystrom is that we're trying to move away from using package >> roots. Modifying the main sdk/bin/dart script to add an implicit >> package root seems antithetical to that aim. >> > >I think this only applies to programs launched through pub. But pub itself >isn't launched through pub. So it really isn't a primitive that you can use >during testing and development of the SDK itself. > > >> What's your chief concern about adding code to >> sdk/bin/dartanalyzer that locates the package root? Are you >> trying to avoid placing this logic in scripts that we ship? >> > >Yes. > > >> If so, how about if I move this logic to a new >> script (called "sdk/bin/dartanalyzer_local", for example), which >> the test infrastructure could execute instead of calling >> dartanalyzer directly? dartanalyzer_local would locate the >> package root, create the "--package-root=*" flag, and store it in >> DART_VM_OPTIONS. Then it would execute sdk/bin/dartanalyzer. I >> think that would address both your concerns and mine. >> > >The purpose of putting scripts in sdk/bin is that the layout reflects the >built SDK, so that you can run the tool without rebuilding the SDK. The >provides fast development. > >If you put a script in sdk/bin, it should work when invoked from there. If >the script contains code for looking up stuff in our build system, it >shouldn't be copied directly to the bin directory in the built SDK. Agreed. To clarify my proposal about "dartanalyzer_local" above, dartanalyzer_local would *not* be copied to the bin directory in the built SDK. (Much in the same way that the "_developer" scripts aren't copied to the bin directory in the built SDK). So I believe this proposal would satisfy your criteria--all the code for looking up stuff in our build system would be confined to scripts that do not ship. Would that be acceptable to you?
On 2014/10/09 13:22:52, Paul Berry wrote: > Agreed. To clarify my proposal about "dartanalyzer_local" above, > dartanalyzer_local would *not* be copied to the bin directory in > the built SDK. (Much in the same way that the "_developer" > scripts aren't copied to the bin directory in the built SDK). So > I believe this proposal would satisfy your criteria--all the code > for looking up stuff in our build system would be confined to > scripts that do not ship. > > Would that be acceptable to you? But the dartanalyzer script would still be in sdk/bin where it doesn't belong. Once you remove it from there, then why have a script called dartanalyzer_local?
On 2014/10/09 13:31:40, ahe wrote: > On 2014/10/09 13:22:52, Paul Berry wrote: > > Agreed. To clarify my proposal about "dartanalyzer_local" above, > > dartanalyzer_local would *not* be copied to the bin directory in > > the built SDK. (Much in the same way that the "_developer" > > scripts aren't copied to the bin directory in the built SDK). So > > I believe this proposal would satisfy your criteria--all the code > > for looking up stuff in our build system would be confined to > > scripts that do not ship. > > > > Would that be acceptable to you? > > But the dartanalyzer script would still be in sdk/bin where it doesn't belong. > Once you remove it from there, then why have a script called dartanalyzer_local? Ok, if I'm understanding correctly, your proposal is: copy the existing sdk/bin/dartanalyzer script to some new location and update the build process to copy it from that new location to the built SDK's bin directory. Once that's done it's ok to modify sdk/bin/dartanalyzer to look up stuff in our build system, because that script isn't shipped anymore, and there's no need to call it "dartanalyzer_local". I can go along with that plan. Where do you think a good location for that new copy of dartanalyzer to go? (The one that the build process will copy to the built SDK's bin directory) This will be the first file of this sort (AFAICT all of the other scripts that we ship are copied from sdk/bin/) so it probably should be a brand new directory.
On 2014/10/09 14:02:50, Paul Berry wrote: > Ok, if I'm understanding correctly, your proposal is: copy the existing > sdk/bin/dartanalyzer script to some new location and update the build process to > copy it from that new location to the built SDK's bin directory. Once that's > done it's ok to modify sdk/bin/dartanalyzer to look up stuff in our build > system, because that script isn't shipped anymore, and there's no need to call > it "dartanalyzer_local". Yes. > I can go along with that plan. Where do you think a good location for that new > copy of dartanalyzer to go? (The one that the build process will copy to the > built SDK's bin directory) This will be the first file of this sort (AFAICT all > of the other scripts that we ship are copied from sdk/bin/) so it probably > should be a brand new directory. There's been some confusion about when to put scripts in sdk/bin/. That directory isn't a template for the bin/ directory in the SDK product. It is a directory which enables incremental development. The Java-based analyzer script shouldn't have been there, because it never worked when run from that location. utils/dartanalyzer/ is probably a good location for the script that gets shipped with the SDK.
PTAL. I've updated the CL to follow Peter's suggestion. Peter: Rather than place the scripts to be shipped in utils/dartanalyzer, I used the _sdk suffix, since this follows a convention already established by pub_sdk and pub_sdk.bat. Ivan: I've removed the extra VM flags for now--this means there will be no change in what we ship. I'll coordinate with Konstantin to see if they're still needed for testing, and I'll follow up with you if necessary.
On 2014/10/09 17:26:15, Paul Berry wrote: > PTAL. I've updated the CL to follow Peter's suggestion. > > Ivan: I've removed the extra VM flags for now--this means there will be no > change in what we ship. I'll coordinate with Konstantin to see if they're still > needed for testing, and I'll follow up with you if necessary. Thanks, much appreciated. -Ivan
Rico and Peter, Any chance you could have a look at this soon? Thanks, Paul
Didn't look at shell and bat files, rest of CL LGTM! https://codereview.chromium.org/628363002/diff/80001/tools/create_sdk.py File tools/create_sdk.py (right): https://codereview.chromium.org/628363002/diff/80001/tools/create_sdk.py#newc... tools/create_sdk.py:115: for executable in ['dart2js', 'dartanalyzer_sdk', 'dartfmt', 'docgen', 'pub_sdk']: Long line. You can break the line anywhere within [ ... ] as Python will continue parsing the statement when there is an open parenthesis-like thing. https://codereview.chromium.org/628363002/diff/80001/tools/testing/dart/compi... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/628363002/diff/80001/tools/testing/dart/compi... tools/testing/dart/compiler_configuration.dart:405: // if both useSdk and isHostChecked are true. Report an error?
I second what Peter said about having scripts that are in sdk/bin be runable from there - but since I also value consistency and I don't want to require you do move the pub files, LGTM Please validate that everything is working as expected with the bat files https://codereview.chromium.org/628363002/diff/80001/sdk/bin/dartanalyzer_sdk File sdk/bin/dartanalyzer_sdk (right): https://codereview.chromium.org/628363002/diff/80001/sdk/bin/dartanalyzer_sdk... sdk/bin/dartanalyzer_sdk:2: # Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file new file so 2014 https://codereview.chromium.org/628363002/diff/80001/sdk/bin/dartanalyzer_sdk... File sdk/bin/dartanalyzer_sdk.bat (right): https://codereview.chromium.org/628363002/diff/80001/sdk/bin/dartanalyzer_sdk... sdk/bin/dartanalyzer_sdk.bat:2: REM Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file new file so 2014 https://codereview.chromium.org/628363002/diff/80001/tools/testing/dart/compi... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/628363002/diff/80001/tools/testing/dart/compi... tools/testing/dart/compiler_configuration.dart:405: // if both useSdk and isHostChecked are true. On 2014/10/15 08:44:54, ahe wrote: > Report an error? +, just do it instead of the comment - it will be shorter anyway
https://codereview.chromium.org/628363002/diff/80001/sdk/bin/dartanalyzer_sdk File sdk/bin/dartanalyzer_sdk (right): https://codereview.chromium.org/628363002/diff/80001/sdk/bin/dartanalyzer_sdk... sdk/bin/dartanalyzer_sdk:2: # Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2014/10/15 11:12:33, ricow1 wrote: > new file so 2014 That seems disingenuous to me, since the file was simply moved--not a single line of it is new. If you feel strongly that it really needs to be changed, let me know and I'll change it. https://codereview.chromium.org/628363002/diff/80001/tools/create_sdk.py File tools/create_sdk.py (right): https://codereview.chromium.org/628363002/diff/80001/tools/create_sdk.py#newc... tools/create_sdk.py:115: for executable in ['dart2js', 'dartanalyzer_sdk', 'dartfmt', 'docgen', 'pub_sdk']: On 2014/10/15 08:44:54, ahe wrote: > Long line. You can break the line anywhere within [ ... ] as Python will > continue parsing the statement when there is an open parenthesis-like thing. Done. https://codereview.chromium.org/628363002/diff/80001/tools/testing/dart/compi... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/628363002/diff/80001/tools/testing/dart/compi... tools/testing/dart/compiler_configuration.dart:405: // if both useSdk and isHostChecked are true. On 2014/10/15 11:12:34, ricow1 wrote: > On 2014/10/15 08:44:54, ahe wrote: > > Report an error? > +, just do it instead of the comment - it will be shorter anyway Done.
johnniwinther@google.com changed reviewers: + johnniwinther@google.com
.bat files LGTM (i.e. works) when modified according the comment. aprelev@: Could you take a look at the .bat files? https://codereview.chromium.org/628363002/diff/160001/sdk/bin/dartanalyzer.bat File sdk/bin/dartanalyzer.bat (right): https://codereview.chromium.org/628363002/diff/160001/sdk/bin/dartanalyzer.ba... sdk/bin/dartanalyzer.bat:48: set PACKAGE_ROOT=%BUILD_DIR%\packages\ Remove the trailing slash since in creates an escaped quote, \", when quoted below.
https://codereview.chromium.org/628363002/diff/160001/sdk/bin/dartanalyzer.bat File sdk/bin/dartanalyzer.bat (right): https://codereview.chromium.org/628363002/diff/160001/sdk/bin/dartanalyzer.ba... sdk/bin/dartanalyzer.bat:48: set PACKAGE_ROOT=%BUILD_DIR%\packages\ On 2014/10/16 11:42:36, Johnni Winther wrote: > Remove the trailing slash since in creates an escaped quote, \", when quoted > below. Done.
Message was sent while issue was closed.
Committed patchset #7 (id:160001) manually as 41150 (presubmit successful).
Message was sent while issue was closed.
On 2014/10/16 11:42:36, Johnni Winther wrote:
> .bat files LGTM (i.e. works) when modified according the comment.
>
> aprelev@: Could you take a look at the .bat files?
Yes, looks good to me too.
Just curious why sdk/bin/dartanalyzer_sdk.bat doesn't have changes introduced
into sdk/bin/dartanalyzer.bat?
If I understand correctly it's dartanalyzer_sdk.bat that gets included into
{build}/{version}/dart-sdk/bin.
Who is the user of this sdk/bin/dartanalyzer.bat vs dartanalyzer_sdk.bat?
Message was sent while issue was closed.
On 2014/10/16 18:22:25, aam wrote:
> On 2014/10/16 11:42:36, Johnni Winther wrote:
> > .bat files LGTM (i.e. works) when modified according the comment.
> >
> > aprelev@: Could you take a look at the .bat files?
>
> Yes, looks good to me too.
Thanks!
>
> Just curious why sdk/bin/dartanalyzer_sdk.bat doesn't have changes introduced
> into sdk/bin/dartanalyzer.bat?
> If I understand correctly it's dartanalyzer_sdk.bat that gets included into
> {build}/{version}/dart-sdk/bin.
>
> Who is the user of this sdk/bin/dartanalyzer.bat vs dartanalyzer_sdk.bat?
Correct; dartanalyzer_sdk.bat is what gets included into
{build}/{version}/dart-sdk/bin (which is what gets released). The changes I'm
making in this CL are just to support the buildbots and developers of the Dart
SDK. For example, developers want to be able to run the analyzer from raw
source rather than from the snapshot, so that they don't have to do a build
before they can test their changes. That feature isn't necessary for the end
user (in fact it would be problematic to ship it to them, since they may not
have the analyzer source code at all).
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
