Replace Java based analyzer with Dart based analyzer when building SDK

Replace Java based analyzer with Dart based analyzer when building SDK
* include analyzer dart files and snapshot in SDK
* modified bin/dartanalyzer to call dart based analyzer
* removed jar files in utils directory

BUG=dartbug.com/18289
R=ricow@google.com, scheglov@google.com

Committed: https://code.google.com/p/dart/source/detail?r=35604

[scheglov, 2014-04-17 20:50:55]

LGTM

I will try it on Windows once it landed and built.

[ahe, 2014-04-22 09:38:13]

This doesn't look good to me.

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer
File sdk/bin/dartanalyzer (right):

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer#newcode6
sdk/bin/dartanalyzer:6: # Run dartanalyzer.dart on the Dart VM. This script
assumes the Dart SDK's directory
Long line.

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer#newcode43
sdk/bin/dartanalyzer:43: BUILD_DIR="$SDK_DIR/../out/$DART_CONFIGURATION"
Why do you need this code? dart2js doesn't. There is a file right next to this
script called "dart", which encapsulates this feature.

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer.bat
File sdk/bin/dartanalyzer.bat (right):

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer.bat#new...
sdk/bin/dartanalyzer.bat:20: set
DARTANALYZER=%SDK_DIR%\..\packages\analyzer\bin\analyzer.dart
I don't think this works.

https://codereview.chromium.org/240723006/diff/1/tools/create_sdk.py
File tools/create_sdk.py (right):

https://codereview.chromium.org/240723006/diff/1/tools/create_sdk.py#newcode115
tools/create_sdk.py:115: for snapshot in ['dart2js', 'dartanalyzer', 'dartfmt',
'utils_wrapper', 'pub']:
Long line.

https://codereview.chromium.org/240723006/diff/1/tools/create_sdk.py#newcode222
tools/create_sdk.py:222: DARTANALYZER = join(SDK_tmp, 'lib', '_internal',
'dartanalyzer')
Why is this needed?

[danrubel, 2014-04-22 18:37:53]

Thanks for looking over all this!

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer
File sdk/bin/dartanalyzer (right):

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer#newcode6
sdk/bin/dartanalyzer:6: # Run dartanalyzer.dart on the Dart VM. This script
assumes the Dart SDK's directory
On 2014/04/22 09:38:13, ahe wrote:
> Long line.

Done.

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer#newcode43
sdk/bin/dartanalyzer:43: BUILD_DIR="$SDK_DIR/../out/$DART_CONFIGURATION"
On 2014/04/22 09:38:13, ahe wrote:
> Why do you need this code? dart2js doesn't. There is a file right next to this
> script called "dart", which encapsulates this feature.

There are two modes supported here.
1) In a "production" SDK the snapshot exists and we run that snapshot.
2) When a launched from the SVN tree (analyzer development in HEAD) there is no
snapshot and we want to reference and launch sources and package from the tree
itself.

You are correct that some of this logic exists in the dart script that resides
next to this script, but that dart script does not contain a reference to the
"--package-root" directory so the code above is still needed.

All this said, the code above and below to launch dart from the SVN tree with a
package root appears in pub, dartfmt, and here. Seems like a good candidate for
refactoring in a subsequent CL. It would be nice if we could roll --package-root
into the dart script, but not sure that make sense for other dart apps.

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer.bat
File sdk/bin/dartanalyzer.bat (right):

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer.bat#new...
sdk/bin/dartanalyzer.bat:20: set
DARTANALYZER=%SDK_DIR%\..\packages\analyzer\bin\analyzer.dart
On 2014/04/22 09:38:13, ahe wrote:
> I don't think this works.

Good point. I copied this from dartfmt.bat so it probably needs to be fixed as
well. I'll have someone with Windows box test this and I'll modify as needed.

https://codereview.chromium.org/240723006/diff/1/tools/create_sdk.py
File tools/create_sdk.py (right):

https://codereview.chromium.org/240723006/diff/1/tools/create_sdk.py#newcode115
tools/create_sdk.py:115: for snapshot in ['dart2js', 'dartanalyzer', 'dartfmt',
'utils_wrapper', 'pub']:
On 2014/04/22 09:38:13, ahe wrote:
> Long line.

Done.

https://codereview.chromium.org/240723006/diff/1/tools/create_sdk.py#newcode222
tools/create_sdk.py:222: DARTANALYZER = join(SDK_tmp, 'lib', '_internal',
'dartanalyzer')
On 2014/04/22 09:38:13, ahe wrote:
> Why is this needed?

Good point. We run the snapshot so we don't need it for running the analyzer. I
was assuming that the end user would like to see the source, but maybe that's
not a good reason. I'll remove it.

[ahe, 2014-04-23 09:15:05]

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer
File sdk/bin/dartanalyzer (right):

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer#newcode43
sdk/bin/dartanalyzer:43: BUILD_DIR="$SDK_DIR/../out/$DART_CONFIGURATION"
On 2014/04/22 18:37:54, danrubel wrote:
> On 2014/04/22 09:38:13, ahe wrote:
> > Why do you need this code? dart2js doesn't. There is a file right next to
this
> > script called "dart", which encapsulates this feature.
> 
> There are two modes supported here.
> 1) In a "production" SDK the snapshot exists and we run that snapshot.
> 2) When a launched from the SVN tree (analyzer development in HEAD) there is
no
> snapshot and we want to reference and launch sources and package from the tree
> itself.
> 
> You are correct that some of this logic exists in the dart script that resides
> next to this script, but that dart script does not contain a reference to the
> "--package-root" directory so the code above is still needed.
> 
> All this said, the code above and below to launch dart from the SVN tree with
a
> package root appears in pub, dartfmt, and here.

Yes. That's because nobody asked my opinion.

There are two alternatives to this:

1. Create as symlink from pkg/analyzer/bin/packages to 
xcodebuild/ReleaseIA32/packages.

2. Extend the "dart" script to support an environment variable such as
"DART_USE_BUILD_PACKAGES".

> Seems like a good candidate for
> refactoring in a subsequent CL.

I'm not so sure about this. I think this code is brittle, and doesn't belong in
scripts we include in the SDK.

> It would be nice if we could roll --package-root
> into the dart script, but not sure that make sense for other dart apps.

I think it makes sense for all Dart programs in this directory except for
dart2js.

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer
File sdk/bin/dartanalyzer (right):

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer#new...
sdk/bin/dartanalyzer:46: DART="$BUILD_DIR/dart-sdk/bin/dart"
This doesn't match what the "dart2js" script does, and I suspect it is wrong.

You need to pass the --package-root, that doesn't imply you shouldn't use the
"dart" script, and shouldn't try to be similar to the "dart2js" script.

[ricow1, 2014-04-23 17:36:18]

+ I vote for Peters 2 suggestion in his last reply

https://codereview.chromium.org/240723006/diff/20001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/240723006/diff/20001/dart.gyp#newcode51
dart.gyp:51: 'target_name': 'dartanalyzer',
you would want another target here for the java based analyzer, otherwise we
can't build it and run tests on the bots - which we still want right

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer
File sdk/bin/dartanalyzer (right):

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer#new...
sdk/bin/dartanalyzer:6: # Run dartanalyzer.dart on the Dart VM. This script
assumes the Dart SDK's directory
long line

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer#new...
sdk/bin/dartanalyzer:46: DART="$BUILD_DIR/dart-sdk/bin/dart"
On 2014/04/23 09:15:05, ahe wrote:
> This doesn't match what the "dart2js" script does, and I suspect it is wrong.
> 
> You need to pass the --package-root, that doesn't imply you shouldn't use the
> "dart" script, and shouldn't try to be similar to the "dart2js" script.
I agree with Peter here, I would copy the functionality that we have in the
dart2js script

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer.bat
File sdk/bin/dartanalyzer.bat (right):

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer.bat...
sdk/bin/dartanalyzer.bat:38: set current=
I pretty much suck at bat scripts - could you add a comment about what is
happening here

[ricow1, 2014-04-23 17:37:39]

+whesse who may know more on bat than me

[Bill Hesse, 2014-04-23 17:43:35]

On 2014/04/23 17:37:39, ricow1 wrote:
> +whesse who may know more on bat than me

I don't know bat language much better than Rico.  ^o^   ^o^   ^o^

[aam-me, 2014-04-23 19:28:50]

Hoping to add some information regarding follow_links snippet in
dartanalyzer.bat :-)

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer.bat
File sdk/bin/dartanalyzer.bat (right):

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer.bat...
sdk/bin/dartanalyzer.bat:38: set current=
https://chromiumcodereview.appspot.com/11740040/ has short exchange regarding
what the code below does:

This is what "dir /a:l" output looks like when you have symbolic
links(<JUNCTION>s):
===
C:\def ghi>dir /a:l 
 Volume in drive C has no label.
 Volume Serial Number is 9401-7D51

 Directory of C:\

01/03/2013  10:11 PM    <JUNCTION>     abc def
[c:\dart_bleeding\dart-repo.9\dart\build\ReleaseIA32\dart-sdk]
01/03/2013  10:56 PM    <JUNCTION>     def ghi
[c:\dart_bleeding\dart-repo.9\dart\build\ReleaseIA32\dart-sdk\bin]
===

So in the output of 'dir /a:l "targetdir"' we are looking for a filename
surrounded by right angle bracket and left square bracket. Once we get that
filename, which is name of the link, we recursively follow that.

Carets are escape characters in this context.

[kustermann, 2014-04-24 07:50:38]

Just skipped over the changes, since rico and peter took already a look.

The duplication of determining the build directory is not really nice, as was
already mentioned.

Looking forward to getting rid of the java-based analyzer :)

https://codereview.chromium.org/240723006/diff/20001/utils/dartanalyzer/.giti...
File utils/dartanalyzer/.gitignore (right):

https://codereview.chromium.org/240723006/diff/20001/utils/dartanalyzer/.giti...
utils/dartanalyzer/.gitignore:2: /dartanalyzer.target.mk
Can't we put this in the repository-root .gitignore with a wildcard pattern. (We
never want to check *Makefile or *.mk files in, right?)

[danrubel, 2014-04-29 16:10:39]

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer
File sdk/bin/dartanalyzer (right):

https://codereview.chromium.org/240723006/diff/1/sdk/bin/dartanalyzer#newcode43
sdk/bin/dartanalyzer:43: BUILD_DIR="$SDK_DIR/../out/$DART_CONFIGURATION"
On 2014/04/23 09:15:05, ahe wrote:
> > Seems like a good candidate for
> > refactoring in a subsequent CL.
> 
> I'm not so sure about this. I think this code is brittle, and doesn't belong
in
> scripts we include in the SDK.

Good point. It's probably in pub and dartfmt for historical reasons but does not
need to be. We are always running the snapshot version of analyzer from a built
SDK. I'll move the non-snapshot portion of the script to a separate file that
does not get copied into the SDK, and then do the same with the WIndows version
of this script.

https://codereview.chromium.org/240723006/diff/20001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/240723006/diff/20001/dart.gyp#newcode51
dart.gyp:51: 'target_name': 'dartanalyzer',
On 2014/04/23 17:36:18, ricow1 wrote:
> you would want another target here for the java based analyzer, otherwise we
> can't build it and run tests on the bots - which we still want right

Assuming that the bots call the dartanalyzer script rather than trying to use
the dartanalyzer.jar file directly, we can replace the Java based analyzer with
the Dart based analyzer and the bots should run as expected... unless you know
of a specific situation where this is not the case.

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer
File sdk/bin/dartanalyzer (right):

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer#new...
sdk/bin/dartanalyzer:6: # Run dartanalyzer.dart on the Dart VM. This script
assumes the Dart SDK's directory
On 2014/04/23 17:36:18, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer#new...
sdk/bin/dartanalyzer:46: DART="$BUILD_DIR/dart-sdk/bin/dart"
On 2014/04/23 17:36:18, ricow1 wrote:
> On 2014/04/23 09:15:05, ahe wrote:
> > This doesn't match what the "dart2js" script does, and I suspect it is
wrong.
> > 
> > You need to pass the --package-root, that doesn't imply you shouldn't use
the
> > "dart" script, and shouldn't try to be similar to the "dart2js" script.
> I agree with Peter here, I would copy the functionality that we have in the
> dart2js script

Since we are always running the snapshot version in the built SDK, I've moved
this code into a separate file and into another CL.

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer.bat
File sdk/bin/dartanalyzer.bat (right):

https://codereview.chromium.org/240723006/diff/20001/sdk/bin/dartanalyzer.bat...
sdk/bin/dartanalyzer.bat:38: set current=
On 2014/04/23 19:28:51, aam wrote:
> https://chromiumcodereview.appspot.com/11740040/ has short exchange regarding
> what the code below does:
> 
> This is what "dir /a:l" output looks like when you have symbolic
> links(<JUNCTION>s):
> ===
> C:\def ghi>dir /a:l 
>  Volume in drive C has no label.
>  Volume Serial Number is 9401-7D51
> 
>  Directory of C:\
> 
> 01/03/2013  10:11 PM    <JUNCTION>     abc def
> [c:\dart_bleeding\dart-repo.9\dart\build\ReleaseIA32\dart-sdk]
> 01/03/2013  10:56 PM    <JUNCTION>     def ghi
> [c:\dart_bleeding\dart-repo.9\dart\build\ReleaseIA32\dart-sdk\bin]
> ===
> 
> So in the output of 'dir /a:l "targetdir"' we are looking for a filename
> surrounded by right angle bracket and left square bracket. Once we get that
> filename, which is name of the link, we recursively follow that.
> 
> Carets are escape characters in this context.

Thank you! I've rolled this comment into the script.

https://codereview.chromium.org/240723006/diff/20001/utils/dartanalyzer/.giti...
File utils/dartanalyzer/.gitignore (right):

https://codereview.chromium.org/240723006/diff/20001/utils/dartanalyzer/.giti...
utils/dartanalyzer/.gitignore:2: /dartanalyzer.target.mk
On 2014/04/24 07:50:38, kustermann wrote:
> Can't we put this in the repository-root .gitignore with a wildcard pattern.
(We
> never want to check *Makefile or *.mk files in, right?)

I'm following the pattern established in compiler, pub, and dartfmt sibling
directories, assuming those are the correct patterns to follow. If it makes
sense to refactor all of those into the repository-root .gitignore, then let me
know and we can tackle that in a separate CL.

[ricow1, 2014-04-30 07:10:20]

https://codereview.chromium.org/240723006/diff/20001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/240723006/diff/20001/dart.gyp#newcode51
dart.gyp:51: 'target_name': 'dartanalyzer',
On 2014/04/29 16:10:40, danrubel wrote:
> On 2014/04/23 17:36:18, ricow1 wrote:
> > you would want another target here for the java based analyzer, otherwise we
> > can't build it and run tests on the bots - which we still want right
> 
> Assuming that the bots call the dartanalyzer script rather than trying to use
> the dartanalyzer.jar file directly, we can replace the Java based analyzer
with
> the Dart based analyzer and the bots should run as expected... unless you know
> of a specific situation where this is not the case.

That is all fine, not my point :-)
My point is, when we land this we will be running tests on the buildbot on the
dart based analyzer exclusively.
Current setup:
  Java based analyzer running on the main waterfall
  Dart based analyzer running on the fyi waterfall
With this:
  Dart based analyzer running on the main waterfall
  Dart based analyzer running on the fyi waterfall
So we are loosing coverage on the java based analyzer. Since the java based
analyzer is still in the editor we need to keep running it on all our tests
until we have eliminated it completely (I vote for tomorrow - but I guess not
:-))
If we don't do this, we could end up in a situation where the dart based
analyzer is not agreeing with the java based analyzer -> if people have
automated testing using the commandline analyzer this is very bad since the
editor and the command line does not agree.

[danrubel, 2014-04-30 17:33:23]

On 2014/04/30 07:10:20, ricow1 wrote:
> https://codereview.chromium.org/240723006/diff/20001/dart.gyp
> File dart.gyp (right):
> 
> https://codereview.chromium.org/240723006/diff/20001/dart.gyp#newcode51
> dart.gyp:51: 'target_name': 'dartanalyzer',
> On 2014/04/29 16:10:40, danrubel wrote:
> > On 2014/04/23 17:36:18, ricow1 wrote:
> > > you would want another target here for the java based analyzer, otherwise
we
> > > can't build it and run tests on the bots - which we still want right
> > 
> > Assuming that the bots call the dartanalyzer script rather than trying to
use
> > the dartanalyzer.jar file directly, we can replace the Java based analyzer
> with
> > the Dart based analyzer and the bots should run as expected... unless you
know
> > of a specific situation where this is not the case.
> 
> That is all fine, not my point :-)
> My point is, when we land this we will be running tests on the buildbot on the
> dart based analyzer exclusively.
> Current setup:
>   Java based analyzer running on the main waterfall
>   Dart based analyzer running on the fyi waterfall
> With this:
>   Dart based analyzer running on the main waterfall
>   Dart based analyzer running on the fyi waterfall
> So we are loosing coverage on the java based analyzer. Since the java based
> analyzer is still in the editor we need to keep running it on all our tests
> until we have eliminated it completely (I vote for tomorrow - but I guess not
> :-))
> If we don't do this, we could end up in a situation where the dart based
> analyzer is not agreeing with the java based analyzer -> if people have
> automated testing using the commandline analyzer this is very bad since the
> editor and the command line does not agree.

Ah! Very good point. We need coverage of both the Java and Dart based analyzers
for the foreseeable future.

Do you mind if that support lands in a subsequent CL?
I'm trying to decouple as much as possible so that I can make some forward
progress in the near term.

Can we move forward with this CL as it stands?

[ricow1, 2014-04-30 17:38:34]

Since this is holding up progress for editor people:
LGTM

We should make another target that builds the java based analyzer, change the
bots so that we have coverage on both.

[danrubel, 2014-04-30 23:09:41]

Committed patchset #4 manually as r35604 (presubmit successful).