Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799303003:
Update third_party/checkstyle to 7.6.1 (Closed)
DescriptionUpdate third_party/checkstyle to 7.6.1
This CL updates third_party/checkstyle to 7.6.1 so checkstyle would
work with Java 8 lambda features.
BUG=703238
Review-Url: https://codereview.chromium.org/2799303003
Cr-Commit-Position: refs/heads/master@{#462968}
Committed: https://chromium.googlesource.com/chromium/src/+/dbce02f63421f7d446d6ef317b456f6ae911f63b
Patch Set 1 : Update checkstyle to 7.6.1 #
Total comments: 6
Patch Set 2 : addressing comments #
Total comments: 3
Messages
Total messages: 37 (20 generated)
Description was changed from ========== Update checkstyle to 7.6.1 BUG= ========== to ========== Update third_party/checkstyle to 7.6.1 This CL updates third_party/checkstyle to 7.6.1 so checkstyle would work with Java 8 lambda features. BUG=703238 ==========
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zpeng@chromium.org changed reviewers: + agrieve@chromium.org
Hi Andrew, PTAL. Thanks!
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org, klobag@chromium.org, nyquist@chromium.org
Tried it out on my java8 patch, and it worked great! lgtm Adding other new OWNERS to R=, as well as Grace for third_party approval.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jbudorick@google.com changed reviewers: + jbudorick@google.com
Could we manage this jar w/ https://codesearch.chromium.org/chromium/src/build/android/update_deps/update... rather than checking it directly into the repo?
On 2017/04/06 20:59:13, wrong jbudorick wrote: > Could we manage this jar w/ > https://codesearch.chromium.org/chromium/src/build/android/update_deps/update... > rather than checking it directly into the repo? I'm confused. The original jar was checked directly into the repo. This CL removes the original jar, uploads the new jar into Google Storage, and is uploading a SHA1 file. Should we do it in another way? Thanks.
On 2017/04/06 22:13:35, F wrote: > On 2017/04/06 20:59:13, wrong jbudorick wrote: > > Could we manage this jar w/ > > > https://codesearch.chromium.org/chromium/src/build/android/update_deps/update... > > rather than checking it directly into the repo? > > I'm confused. The original jar was checked directly into the repo. This CL > removes the original jar, uploads the new jar into Google Storage, and is > uploading a SHA1 file. Should we do it in another way? Thanks. erp, that's what I get for doing this review too quickly -- misread the list of files. Sorry about that! lgtm
Could you update the README-instructions to include how to update this with the command line tool?
On 2017/04/06 23:01:27, nyquist wrote: > Could you update the README-instructions to include how to update this with the > command line tool? After this CL, checkstyle is maintained following the procedure documented in "Moving large files to Google Storage" https://sites.google.com/a/google.com/chrome-infrastructure/platform/git-conv... Lots of third_party/ files follow this paradigm: store the jar in Google storage and use gclient sync to download the jar when necessary. This page is internal only and is referenced by the markdown "Adding third_party library" https://chromium.googlesource.com/chromium/src.git/+/master/docs/adding_to_th... I don't think it very helpful to include this page in the readme. WDUT?
On 2017/04/06 23:56:06, F wrote: > On 2017/04/06 23:01:27, nyquist wrote: > > Could you update the README-instructions to include how to update this with > the > > command line tool? > > After this CL, checkstyle is maintained following the procedure documented in > "Moving large files to Google Storage" > https://sites.google.com/a/google.com/chrome-infrastructure/platform/git-conv... > > Lots of third_party/ files follow this paradigm: store the jar in Google storage > and use gclient sync to download the jar when necessary. > > This page is internal only and is referenced by the markdown "Adding third_party > library" > https://chromium.googlesource.com/chromium/src.git/+/master/docs/adding_to_th... > > I don't think it very helpful to include this page in the readme. WDUT? Replied in-line in readme.md
https://codereview.chromium.org/2799303003/diff/20001/third_party/checkstyle/... File third_party/checkstyle/README.chromium (right): https://codereview.chromium.org/2799303003/diff/20001/third_party/checkstyle/... third_party/checkstyle/README.chromium:4: URL: http://checkstyle.sourceforge.net/ Could we update this to https://github.com/checkstyle/checkstyle in fact? https://codereview.chromium.org/2799303003/diff/20001/third_party/checkstyle/... third_party/checkstyle/README.chromium:10: Description: That page you mentioned contains a lot of generic information. I think you can skip the part about creating it, and probably just quickly mention that you need to use your @google.com account for the auth (just mention `download_from_google_storage --config`). Then, the last thing is to in fact update the file, where you could in particular specify the exact command line you need to run. Otherwise developers would have to look in the DEPS-file to find the bucket name, etc. So yeah, I think it would be helpful to write down the full command for update. https://codereview.chromium.org/2799303003/diff/20001/third_party/checkstyle/... third_party/checkstyle/README.chromium:14: - Downloaded checkstyle-7.6.1-all.jar without source code development Could you mention that this could be downloaded from: https://github.com/checkstyle/checkstyle/releases ?
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Andrew, John, and Tommy! PTAL https://codereview.chromium.org/2799303003/diff/20001/third_party/checkstyle/... File third_party/checkstyle/README.chromium (right): https://codereview.chromium.org/2799303003/diff/20001/third_party/checkstyle/... third_party/checkstyle/README.chromium:4: URL: http://checkstyle.sourceforge.net/ On 2017/04/07 00:10:06, nyquist wrote: > Could we update this to https://github.com/checkstyle/checkstyle in fact? Done. https://codereview.chromium.org/2799303003/diff/20001/third_party/checkstyle/... third_party/checkstyle/README.chromium:10: Description: On 2017/04/07 00:10:06, nyquist wrote: > That page you mentioned contains a lot of generic information. I think you can > skip the part about creating it, and probably just quickly mention that you need > to use your @google.com account for the auth (just mention > `download_from_google_storage --config`). > > Then, the last thing is to in fact update the file, where you could in > particular specify the exact command line you need to run. Otherwise developers > would have to look in the DEPS-file to find the bucket name, etc. > > So yeah, I think it would be helpful to write down the full command for update. Done. https://codereview.chromium.org/2799303003/diff/20001/third_party/checkstyle/... third_party/checkstyle/README.chromium:14: - Downloaded checkstyle-7.6.1-all.jar without source code development On 2017/04/07 00:10:06, nyquist wrote: > Could you mention that this could be downloaded from: > https://github.com/checkstyle/checkstyle/releases ? Done. The fat jar seems to be only available on sourceforge.net
Thanks! lgtm https://codereview.chromium.org/2799303003/diff/40001/third_party/checkstyle/... File third_party/checkstyle/README.chromium (right): https://codereview.chromium.org/2799303003/diff/40001/third_party/checkstyle/... third_party/checkstyle/README.chromium:19: - Remove existing SHA1 file Nit; is this required, or will the call to upload_to_google_storage.py just overwrite it?
Thanks Andrew, John, and Tommy! PTAL https://codereview.chromium.org/2799303003/diff/40001/third_party/checkstyle/... File third_party/checkstyle/README.chromium (right): https://codereview.chromium.org/2799303003/diff/40001/third_party/checkstyle/... third_party/checkstyle/README.chromium:19: - Remove existing SHA1 file On 2017/04/07 18:30:28, nyquist wrote: > Nit; is this required, or will the call to upload_to_google_storage.py just > overwrite it? This is required. The script will generate a new SHA1 file with a different name for a different version of checkstyle.
https://codereview.chromium.org/2799303003/diff/40001/third_party/checkstyle/... File third_party/checkstyle/README.chromium (right): https://codereview.chromium.org/2799303003/diff/40001/third_party/checkstyle/... third_party/checkstyle/README.chromium:19: - Remove existing SHA1 file On 2017/04/07 18:37:40, F wrote: > On 2017/04/07 18:30:28, nyquist wrote: > > Nit; is this required, or will the call to upload_to_google_storage.py just > > overwrite it? > > This is required. The script will generate a new SHA1 file with a different name > for a different version of checkstyle. Doh! Of course! Thanks :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zpeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2799303003/#ps40001 (title: "addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491593038919950,
"parent_rev": "6cd116a5bd21185f22e6231c8b048d898f7119c2", "commit_rev":
"dbce02f63421f7d446d6ef317b456f6ae911f63b"}
Message was sent while issue was closed.
Description was changed from ========== Update third_party/checkstyle to 7.6.1 This CL updates third_party/checkstyle to 7.6.1 so checkstyle would work with Java 8 lambda features. BUG=703238 ========== to ========== Update third_party/checkstyle to 7.6.1 This CL updates third_party/checkstyle to 7.6.1 so checkstyle would work with Java 8 lambda features. BUG=703238 Review-Url: https://codereview.chromium.org/2799303003 Cr-Commit-Position: refs/heads/master@{#462968} Committed: https://chromium.googlesource.com/chromium/src/+/dbce02f63421f7d446d6ef317b45... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/dbce02f63421f7d446d6ef317b45...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/2808553002/ by zpeng@chromium.org. The reason for reverting is: Breaking trybots.
Message was sent while issue was closed.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Message was sent while issue was closed.
I was able to revert via https://codereview.chromium.org/2807753003 Did a local git revert on the original commit hash, then committed that locally, uploaded to codereview and did a git cl land. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
