Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16667023:
Fixed sdk/bin/dart2analyzer to use package-root (Closed)
|
Created:
7 years, 6 months ago by kustermann Modified:
7 years, 6 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionFixed sdk/bin/dart2analyzer to use package-root
R=ahe@google.com, ricow@google.com, scheglov@google.com
Committed: https://code.google.com/p/dart/source/detail?r=23905
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Messages
Total messages: 14 (0 generated)
With this change, I can run $ ./tools/test.py -mrelease -cdart2analyzer -rnone without any issues. As soon as I can land this CL, I can setup a builder on FYI. https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (left): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#oldcode30 sdk/bin/dart2analyzer:30: exec "$DART" "${PKG_ANALYZER}" --dart-sdk "${SDK_DIR}" "$@" Which script passes a boolean '--dart-sdk' flag to sdk/bin/dart2analyer? In case this is not used, we should remove it. https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (right): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode45 sdk/bin/dart2analyzer:45: exec "$BUILD_DIR/dart" "--package-root=$PACKAGE_ROOT" "${PKG_ANALYZER}" "$@" I have no idea how worked without '--package-root'?
LGTM, but a few questions https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (right): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode6 sdk/bin/dart2analyzer:6: # NOTE: This file is not supposed to be included in the SDK. So why don't we move it? https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode45 sdk/bin/dart2analyzer:45: exec "$BUILD_DIR/dart" "--package-root=$PACKAGE_ROOT" "${PKG_ANALYZER}" "$@" On 2013/06/11 10:28:15, kustermann wrote: > I have no idea how worked without '--package-root'? Did it?
https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (right): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode6 sdk/bin/dart2analyzer:6: # NOTE: This file is not supposed to be included in the SDK. On 2013/06/11 10:35:34, ricow1 wrote: > So why don't we move it? Sure. Where should we move it to? https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode45 sdk/bin/dart2analyzer:45: exec "$BUILD_DIR/dart" "--package-root=$PACKAGE_ROOT" "${PKG_ANALYZER}" "$@" On 2013/06/11 10:35:34, ricow1 wrote: > On 2013/06/11 10:28:15, kustermann wrote: > > I have no idea how worked without '--package-root'? > > Did it? It didn't work on my machine. But I expected that the people who made the script tested it?!
On 2013/06/11 10:38:29, kustermann wrote: > https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer > File sdk/bin/dart2analyzer (right): > > https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode6 > sdk/bin/dart2analyzer:6: # NOTE: This file is not supposed to be included in the > SDK. > On 2013/06/11 10:35:34, ricow1 wrote: > > So why don't we move it? > > Sure. Where should we move it to? editor/tools or editor/utils seems like good places, but I will leave this to the editor people to decide. Feel free to land as is > > https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode45 > sdk/bin/dart2analyzer:45: exec "$BUILD_DIR/dart" "--package-root=$PACKAGE_ROOT" > "${PKG_ANALYZER}" "$@" > On 2013/06/11 10:35:34, ricow1 wrote: > > On 2013/06/11 10:28:15, kustermann wrote: > > > I have no idea how worked without '--package-root'? > > > > Did it? > > It didn't work on my machine. But I expected that the people who made the script > tested it?!
https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (left): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#oldcode30 sdk/bin/dart2analyzer:30: exec "$DART" "${PKG_ANALYZER}" --dart-sdk "${SDK_DIR}" "$@" On 2013/06/11 10:28:15, kustermann wrote: > Which script passes a boolean '--dart-sdk' flag to sdk/bin/dart2analyer? > In case this is not used, we should remove it. In general, this is script which users may call to do analysis. So, users may use --dart-sdk. It seems that with there changes we make this script good for running on the bot, but not when it will eventually become part of the SDK. May be worth to keep this script unchanged and apply instead this content to the dart2analyzer_developer. https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (right): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode6 sdk/bin/dart2analyzer:6: # NOTE: This file is not supposed to be included in the SDK. On 2013/06/11 10:38:29, kustermann wrote: > On 2013/06/11 10:35:34, ricow1 wrote: > > So why don't we move it? > > Sure. Where should we move it to? For now yes, this file should not be included in the SDK. However plan is that it will replace dartanalyzer script in some not-so-far future. So, if it does not cause problem with packaging the SDK, I'd like to keep it here.
On 2013/06/11 20:07:53, scheglov wrote: > https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer > File sdk/bin/dart2analyzer (left): > > https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#oldcode30 > sdk/bin/dart2analyzer:30: exec "$DART" "${PKG_ANALYZER}" --dart-sdk "${SDK_DIR}" > "$@" > On 2013/06/11 10:28:15, kustermann wrote: > > Which script passes a boolean '--dart-sdk' flag to sdk/bin/dart2analyer? > > In case this is not used, we should remove it. > > In general, this is script which users may call to do analysis. > So, users may use --dart-sdk. > > It seems that with there changes we make this script good for running on the > bot, but not when it will eventually become part of the SDK. > > May be worth to keep this script unchanged and apply instead this content to the > dart2analyzer_developer. > > https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer > File sdk/bin/dart2analyzer (right): > > https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode6 > sdk/bin/dart2analyzer:6: # NOTE: This file is not supposed to be included in the > SDK. > On 2013/06/11 10:38:29, kustermann wrote: > > On 2013/06/11 10:35:34, ricow1 wrote: > > > So why don't we move it? > > > > Sure. Where should we move it to? > > For now yes, this file should not be included in the SDK. > However plan is that it will replace dartanalyzer script in some not-so-far > future. > So, if it does not cause problem with packaging the SDK, I'd like to keep it > here.
https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (left): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#oldcode30 sdk/bin/dart2analyzer:30: exec "$DART" "${PKG_ANALYZER}" --dart-sdk "${SDK_DIR}" "$@" On 2013/06/11 20:07:53, scheglov wrote: > On 2013/06/11 10:28:15, kustermann wrote: > > Which script passes a boolean '--dart-sdk' flag to sdk/bin/dart2analyer? > > In case this is not used, we should remove it. > > In general, this is script which users may call to do analysis. > So, users may use --dart-sdk. What does that flag do? > > It seems that with there changes we make this script good for running on the > bot, but not when it will eventually become part of the SDK. > When it becomes part of the sdk we need to change this completely. We will snapshot the analyzer and add only the snapshot and small wrapper script that will start it. Take a look at how we do dart2js, dartdoc and pub > May be worth to keep this script unchanged and apply instead this content to the > dart2analyzer_developer. That will work for getting this on the buildbot, but as I noted above this file needs to be changed anyway when we make it part of the SDK. https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (right): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode6 sdk/bin/dart2analyzer:6: # NOTE: This file is not supposed to be included in the SDK. On 2013/06/11 20:07:53, scheglov wrote: > On 2013/06/11 10:38:29, kustermann wrote: > > On 2013/06/11 10:35:34, ricow1 wrote: > > > So why don't we move it? > > > > Sure. Where should we move it to? > > For now yes, this file should not be included in the SDK. > However plan is that it will replace dartanalyzer script in some not-so-far > future. > So, if it does not cause problem with packaging the SDK, I'd like to keep it > here. OK - for future reference please only place stuff in here that is actually shipping in the SDK. It will not interfere with the sdk packaging, but we should try to keep the two as close as possible. There is no (at least I don't see any) reason for having it here. https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode45 sdk/bin/dart2analyzer:45: exec "$BUILD_DIR/dart" "--package-root=$PACKAGE_ROOT" "${PKG_ANALYZER}" "$@" On 2013/06/11 10:38:29, kustermann wrote: > On 2013/06/11 10:35:34, ricow1 wrote: > > On 2013/06/11 10:28:15, kustermann wrote: > > > I have no idea how worked without '--package-root'? > > > > Did it? > > It didn't work on my machine. But I expected that the people who made the script > tested it?! Konstantin: how are you setting the package root without this change?
LGTM https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (left): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#oldcode30 sdk/bin/dart2analyzer:30: exec "$DART" "${PKG_ANALYZER}" --dart-sdk "${SDK_DIR}" "$@" On 2013/06/12 07:54:22, ricow1 wrote: > On 2013/06/11 20:07:53, scheglov wrote: > > On 2013/06/11 10:28:15, kustermann wrote: > > > Which script passes a boolean '--dart-sdk' flag to sdk/bin/dart2analyer? > > > In case this is not used, we should remove it. > > > > In general, this is script which users may call to do analysis. > > So, users may use --dart-sdk. > What does that flag do? It allows to analyze against different SDK than SDK where this script is located. This is how dartc and the new Java based analyzer was done. I don't have strong opinion if this is useful or now. > > > > It seems that with there changes we make this script good for running on the > > bot, but not when it will eventually become part of the SDK. > > > When it becomes part of the sdk we need to change this completely. We will > snapshot the analyzer and add only the snapshot and small wrapper script that > will start it. Take a look at how we do dart2js, dartdoc and pub Yes, I understand. > > > May be worth to keep this script unchanged and apply instead this content to > the > > dart2analyzer_developer. > That will work for getting this on the buildbot, but as I noted above this file > needs to be changed anyway when we make it part of the SDK. OK https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer File sdk/bin/dart2analyzer (right): https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode6 sdk/bin/dart2analyzer:6: # NOTE: This file is not supposed to be included in the SDK. > > For now yes, this file should not be included in the SDK. > > However plan is that it will replace dartanalyzer script in some not-so-far > > future. > > So, if it does not cause problem with packaging the SDK, I'd like to keep it > > here. > OK - for future reference please only place stuff in here that is actually > shipping in the SDK. It will not interfere with the sdk packaging, but we should > try to keep the two as close as possible. There is no (at least I don't see any) > reason for having it here. OK, agree. Lets move it to the appropriate place until it will be actually part of the SDK. https://codereview.chromium.org/16667023/diff/1/sdk/bin/dart2analyzer#newcode45 sdk/bin/dart2analyzer:45: exec "$BUILD_DIR/dart" "--package-root=$PACKAGE_ROOT" "${PKG_ANALYZER}" "$@" On 2013/06/12 07:54:22, ricow1 wrote: > On 2013/06/11 10:38:29, kustermann wrote: > > On 2013/06/11 10:35:34, ricow1 wrote: > > > On 2013/06/11 10:28:15, kustermann wrote: > > > > I have no idea how worked without '--package-root'? > > > > > > Did it? > > > > It didn't work on my machine. But I expected that the people who made the > script > > tested it?! > Konstantin: how are you setting the package root without this change? It seems that it worked only on my machine :-( I've had a link sdk/packages to repo/pkg folder.
PTAL I moved it now to editor/tools/analyzer_experimental. Following that, I'll setup a builder on FYI for it. As soon as it's ready to replace the java analyzer, we'll follow peter's suggestion.
lgtm https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... File editor/tools/analyzer_experimental (right): https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:1: #!/bin/bash --posix What is the --posix option for? https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:8: CUR_DIR="$(cd "${BASH_SOURCE%/*}" ; pwd -P)" Is ${BASH_SOURCE%/*} a POSIX feature? https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:11: then Keep then on same line as if. https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:20: then Ditto. https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:41: exec "$BUILD_DIR/dart" "--package-root=$PACKAGE_ROOT" "${PKG_ANALYZER}" --dart-sdk "${SDK_DIR}" "$@" Long line.
thank you. https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... File editor/tools/analyzer_experimental (right): https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:1: #!/bin/bash --posix On 2013/06/12 13:21:19, ahe wrote: > What is the --posix option for? Left over from old dart2analyzer script. Should not be necessary. https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:8: CUR_DIR="$(cd "${BASH_SOURCE%/*}" ; pwd -P)" On 2013/06/12 13:21:19, ahe wrote: > Is ${BASH_SOURCE%/*} a POSIX feature? I don't know. But sdk/bin/dart2js uses BASH_SOURCE even without '--posix'. I think it's safe to use. https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:11: then On 2013/06/12 13:21:19, ahe wrote: > Keep then on same line as if. Done. https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:20: then On 2013/06/12 13:21:19, ahe wrote: > Ditto. Done. https://codereview.chromium.org/16667023/diff/11001/editor/tools/analyzer_exp... editor/tools/analyzer_experimental:41: exec "$BUILD_DIR/dart" "--package-root=$PACKAGE_ROOT" "${PKG_ANALYZER}" --dart-sdk "${SDK_DIR}" "$@" On 2013/06/12 13:21:19, ahe wrote: > Long line. Done.
Message was sent while issue was closed.
Committed patchset #3 manually as r23905 (presubmit successful).
Message was sent while issue was closed.
LGTM Thank you! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
