Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1462243002:
GN: Support building liblouis library (Closed)
|
Created:
5 years, 1 month ago by Petr Hosek Modified:
5 years ago CC:
chromium-reviews, oshima+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, arv+watch_chromium.org, dtseng+watch_chromium.org, dmazzoni+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN: Support building liblouis library
This library and the table data is a part of Chrome OS.
BUG=512906
Committed: https://crrev.com/67643e64039078d54f4b6842369d33b8ba131fc7
Cr-Commit-Position: refs/heads/master@{#362518}
Patch Set 1 : #
Total comments: 24
Patch Set 2 : Code review feedback incorporated #
Total comments: 2
Patch Set 3 : Reduce visibility even futher #
Messages
Total messages: 24 (11 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== GN: Support building liblouis library BUG=512906 ========== to ========== GN: Support building liblouis library BUG=512906 ==========
phosek@chromium.org changed reviewers: + dpranke@chromium.org, plundblad@chromium.org, sky@chromium.org
Description was changed from ========== GN: Support building liblouis library BUG=512906 ========== to ========== GN: Support building liblouis library This library and the table data is a part of Chrome OS. BUG=512906 ==========
Hi, This is cool! I patched and tested this with chromevox and an actual hardware braille display and it works end to end! Wohoa! https://codereview.chromium.org/1462243002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/BUILD.gn (right): https://codereview.chromium.org/1462243002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/BUILD.gn:395: sources -= [ "braille/liblouis_test.extjs" ] # TODO(GYP) Remove the line that excludes braille/liblouis_test.extjs now that these tests have what they need to run (I patched your cl and tested this). https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... File third_party/liblouis/BUILD.gn (right): https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:8: if (current_toolchain == default_toolchain) { Curious, why do we need this check here? https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:10: chromevox_braille_out_dir = nit: This is only used once, so could just use the value inline. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:19: # dest_dir: destination path for all translator files List 'deps' here as well (no need for a description). Also, add testonly here and forward to the targets. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:29: action(tables_target_name) { Add visibility = [ ":$target_name" ] https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:55: copy(tables_json_target_name) { Add visibility = [ ":$target_name" ] https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:64: copy(nexe_target_name) { Add visibility = [ ":$target_name" ] https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:86: generate_nmf(nmf_target_name) { Add visibility = [ ":$target_name" ] https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:116: } Add testonly = true (see comment on the template itself). https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:118: copy("liblouis_test_files") { Add visibility = ":$liblouis_test_data" and testonly = true https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:144: source_set("liblouis_nacl") { Add visibility = [ ":liblouis_nacl_wrapper" ] https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:164: executable("liblouis_nacl_wrapper") { Add visibility = [ "*" ] or list the two targets.
https://codereview.chromium.org/1462243002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/BUILD.gn (right): https://codereview.chromium.org/1462243002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/BUILD.gn:395: sources -= [ "braille/liblouis_test.extjs" ] # TODO(GYP) On 2015/11/30 10:27:03, Peter Lundblad wrote: > Remove the line that excludes braille/liblouis_test.extjs now that these tests > have what they need to run (I patched your cl and tested this). Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... File third_party/liblouis/BUILD.gn (right): https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:8: if (current_toolchain == default_toolchain) { On 2015/11/30 10:27:04, Peter Lundblad wrote: > Curious, why do we need this check here? To ensure that these targets are not instantiated using the Chromium clang toolchain which leads to an error. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:10: chromevox_braille_out_dir = On 2015/11/30 10:27:04, Peter Lundblad wrote: > nit: This is only used once, so could just use the value inline. Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:19: # dest_dir: destination path for all translator files On 2015/11/30 10:27:04, Peter Lundblad wrote: > List 'deps' here as well (no need for a description). > > Also, add testonly here and forward to the targets. Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:29: action(tables_target_name) { On 2015/11/30 10:27:04, Peter Lundblad wrote: > Add visibility = [ ":$target_name" ] Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:55: copy(tables_json_target_name) { On 2015/11/30 10:27:04, Peter Lundblad wrote: > Add visibility = [ ":$target_name" ] Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:64: copy(nexe_target_name) { On 2015/11/30 10:27:04, Peter Lundblad wrote: > Add visibility = [ ":$target_name" ] Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:86: generate_nmf(nmf_target_name) { On 2015/11/30 10:27:04, Peter Lundblad wrote: > Add visibility = [ ":$target_name" ] Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:116: } On 2015/11/30 10:27:04, Peter Lundblad wrote: > Add testonly = true (see comment on the template itself). Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:118: copy("liblouis_test_files") { On 2015/11/30 10:27:04, Peter Lundblad wrote: > Add > visibility = ":$liblouis_test_data" > and > testonly = true Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:144: source_set("liblouis_nacl") { On 2015/11/30 10:27:04, Peter Lundblad wrote: > Add visibility = [ ":liblouis_nacl_wrapper" ] Done. https://codereview.chromium.org/1462243002/diff/20001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:164: executable("liblouis_nacl_wrapper") { On 2015/11/30 10:27:03, Peter Lundblad wrote: > Add visibility = [ "*" ] or list the two targets. Done.
rubber-stamp lgtm
lgtm https://codereview.chromium.org/1462243002/diff/40001/third_party/liblouis/BU... File third_party/liblouis/BUILD.gn (right): https://codereview.chromium.org/1462243002/diff/40001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:182: visibility = [ "*" ] Sorry, my bad, this should be: visibility = ":*" to limit visibility to targets in this file.
The CQ bit was checked by phosek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, plundblad@chromium.org Link to the patchset: https://codereview.chromium.org/1462243002/#ps60001 (title: "Reduce visibility even futher")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1462243002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1462243002/60001
https://codereview.chromium.org/1462243002/diff/40001/third_party/liblouis/BU... File third_party/liblouis/BUILD.gn (right): https://codereview.chromium.org/1462243002/diff/40001/third_party/liblouis/BU... third_party/liblouis/BUILD.gn:182: visibility = [ "*" ] On 2015/12/01 09:10:33, Peter Lundblad wrote: > Sorry, my bad, this should be: > visibility = ":*" > to limit visibility to targets in this file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
phosek@chromium.org changed reviewers: + phajdan.jr@chromium.org
Scott, Pawel would it be possible to review the chrome/test part?
LGTM
The CQ bit was checked by phosek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1462243002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1462243002/60001
Message was sent while issue was closed.
Description was changed from ========== GN: Support building liblouis library This library and the table data is a part of Chrome OS. BUG=512906 ========== to ========== GN: Support building liblouis library This library and the table data is a part of Chrome OS. BUG=512906 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== GN: Support building liblouis library This library and the table data is a part of Chrome OS. BUG=512906 ========== to ========== GN: Support building liblouis library This library and the table data is a part of Chrome OS. BUG=512906 Committed: https://crrev.com/67643e64039078d54f4b6842369d33b8ba131fc7 Cr-Commit-Position: refs/heads/master@{#362518} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/67643e64039078d54f4b6842369d33b8ba131fc7 Cr-Commit-Position: refs/heads/master@{#362518} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
