Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1497543006:
GN: Build nacl_helper_nonsfi unit tests (Closed)
|
Created:
5 years ago by Petr Hosek Modified:
5 years ago Reviewers:
Dirk Pranke, dpranke, bradnelson, Mark Seaborn, Nico, jln (very slow on Chromium), Roland McGrath, hidehiko, brettw, Sam Clegg CC:
chromium-reviews, rickyz+watch_chromium.org, jln+watch_chromium.org, vmpstr+watch_chromium.org, Sam Clegg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN: Build nacl_helper_nonsfi unit tests
The DoSocketpair function in nonsfi_sandbox_unittest.cc is unused
which causes a build error due to -Wunused-function which is set by
default in GN hence removing the function.
BUG=564877
Committed: https://crrev.com/ca8571a9777c5207d61dc28c323779e767b99bad
Cr-Commit-Position: refs/heads/master@{#365836}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Code review feedback incorporated #
Total comments: 4
Patch Set 3 : Code review feedback incorporated #Patch Set 4 : Define _run target for GYP compatibility #
Total comments: 4
Patch Set 5 : Use += instead of -= #Patch Set 6 : Rebase #
Messages
Total messages: 45 (22 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== GN: Build nacl_helper_nonsfi unit tests BUG=564877 ========== to ========== GN: Build nacl_helper_nonsfi unit tests BUG=564877 ==========
phosek@chromium.org changed reviewers: + dpranke@google.com, mcgrathr@chromium.org, mseaborn@chromium.org
LGTM though I'm not really familiar with the code being built.
https://codereview.chromium.org/1497543006/diff/20001/base/json/BUILD.gn File base/json/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/20001/base/json/BUILD.gn#newc... base/json/BUILD.gn:25: if (is_nacl && !is_nacl_nonsfi) { I don't see this being conditionalised in the Gyp build. If it builds with newlib, can we remove this "source -= ..." for simplicity? https://codereview.chromium.org/1497543006/diff/20001/components/nacl/BUILD.gn File components/nacl/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/20001/components/nacl/BUILD.g... components/nacl/BUILD.gn:365: "${root_build_dir}/{{source_file_part}}", Does {{source_file_part}} expand to "nacl_helper_nonsfi_unittests_main"? If so, would it be clearer to write the latter? https://codereview.chromium.org/1497543006/diff/20001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (left): https://codereview.chromium.org/1497543006/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:116: void DoSocketpair(base::ScopedFD* fds) { Is removing this required because GN is compiling with different warnings? Please comment this part in the commit message. https://codereview.chromium.org/1497543006/diff/20001/sandbox/linux/BUILD.gn File sandbox/linux/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/20001/sandbox/linux/BUILD.gn#... sandbox/linux/BUILD.gn:85: "tests/test_utils.cc", These don't seem to be conditionalised in the Gyp build. Why conditionalise them here?
Description was changed from ========== GN: Build nacl_helper_nonsfi unit tests BUG=564877 ========== to ========== GN: Build nacl_helper_nonsfi unit tests The DoSocketpair function in nonsfi_sandbox_unittest.cc is unused which causes a build error due to -Wunused-function which is set by default in GN hence removing the function. BUG=564877 ==========
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm if mseaborn is happy. I noted a couple of nits ... https://codereview.chromium.org/1497543006/diff/100001/base/json/BUILD.gn File base/json/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/100001/base/json/BUILD.gn#new... base/json/BUILD.gn:5: import("//build/config/nacl/config.gni") why is this import needed? https://codereview.chromium.org/1497543006/diff/100001/base/test/BUILD.gn File base/test/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/100001/base/test/BUILD.gn#new... base/test/BUILD.gn:6: import("//build/config/nacl/config.gni") is this import needed?
https://codereview.chromium.org/1497543006/diff/20001/base/json/BUILD.gn File base/json/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/20001/base/json/BUILD.gn#newc... base/json/BUILD.gn:25: if (is_nacl && !is_nacl_nonsfi) { On 2015/12/03 19:05:07, Mark Seaborn wrote: > I don't see this being conditionalised in the Gyp build. If it builds with > newlib, can we remove this "source -= ..." for simplicity? Unfortunately not, this breaks under newlib because of missing file path support from base. https://codereview.chromium.org/1497543006/diff/20001/components/nacl/BUILD.gn File components/nacl/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/20001/components/nacl/BUILD.g... components/nacl/BUILD.gn:365: "${root_build_dir}/{{source_file_part}}", On 2015/12/03 19:05:07, Mark Seaborn wrote: > Does {{source_file_part}} expand to "nacl_helper_nonsfi_unittests_main"? If so, > would it be clearer to write the latter? Yes and no, this is the preferred GN way. https://codereview.chromium.org/1497543006/diff/20001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (left): https://codereview.chromium.org/1497543006/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:116: void DoSocketpair(base::ScopedFD* fds) { On 2015/12/03 19:05:07, Mark Seaborn wrote: > Is removing this required because GN is compiling with different warnings? > Please comment this part in the commit message. Yes, this function is not being used which triggers a compilation error because of -Wunused-function which is set by default in GN. I have added a comment into the commit message. https://codereview.chromium.org/1497543006/diff/20001/sandbox/linux/BUILD.gn File sandbox/linux/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/20001/sandbox/linux/BUILD.gn#... sandbox/linux/BUILD.gn:85: "tests/test_utils.cc", On 2015/12/03 19:05:07, Mark Seaborn wrote: > These don't seem to be conditionalised in the Gyp build. Why conditionalise > them here? Gyp build is different, it defines and uses custom targets which are usually named same as the regular target with the `_nacl_nonsfi` or `_nonsfi` suffix. However, this is discouraged in GN and the preferred way is to customize the regular target which is what I've been doing already for nacl_helper_nonsfi build. These two files are not part of sandbox_linux_test_utils_nacl_nonsfi and they don't build because of some missing definitions in our C library. They're also not needed for Non-SFI tests hence removing them. https://codereview.chromium.org/1497543006/diff/100001/base/json/BUILD.gn File base/json/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/100001/base/json/BUILD.gn#new... base/json/BUILD.gn:5: import("//build/config/nacl/config.gni") On 2015/12/04 00:22:41, Dirk Pranke wrote: > why is this import needed? This is needed for is_nacl and is_nacl_nonsfi. https://codereview.chromium.org/1497543006/diff/100001/base/test/BUILD.gn File base/test/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/100001/base/test/BUILD.gn#new... base/test/BUILD.gn:6: import("//build/config/nacl/config.gni") On 2015/12/04 00:22:41, Dirk Pranke wrote: > is this import needed? This is for is_nacl_nonsfi.
On 2015/12/04 05:57:46, Petr Hosek wrote: > https://codereview.chromium.org/1497543006/diff/100001/base/test/BUILD.gn#new... > base/test/BUILD.gn:6: import("//build/config/nacl/config.gni") > On 2015/12/04 00:22:41, Dirk Pranke wrote: > > is this import needed? > > This is for is_nacl_nonsfi. Okay, I see that this and the other one are now needed in patchset #3.
Ping, Mark would it be possible to re-review this?
phosek@chromium.org changed reviewers: + hidehiko@chromium.org, jln@chromium.org
On 2015/12/07 21:50:42, Petr Hosek wrote: > Ping, Mark would it be possible to re-review this? Julien or Hidehiko, would it be possible to do OWNERS review of this? Mark is unresponsive and there are people who need this patch to land ASAP.
lgtm for sandbox stuff with a nit. https://codereview.chromium.org/1497543006/diff/140001/sandbox/linux/BUILD.gn File sandbox/linux/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/140001/sandbox/linux/BUILD.gn... sandbox/linux/BUILD.gn:68: if (use_seccomp_bpf || is_nacl_nonsfi) { nit: it would be better if is_nacl_nonsfi would imply use_seccomp_bpf instead. No big deal and no need to do that in this CL. https://codereview.chromium.org/1497543006/diff/140001/sandbox/linux/BUILD.gn... sandbox/linux/BUILD.gn:84: sources -= [ I'm a bit alergic to removing files, it makes things hard to understand. Do you mind doing a conditional += above instead?
https://codereview.chromium.org/1497543006/diff/140001/sandbox/linux/BUILD.gn File sandbox/linux/BUILD.gn (right): https://codereview.chromium.org/1497543006/diff/140001/sandbox/linux/BUILD.gn... sandbox/linux/BUILD.gn:68: if (use_seccomp_bpf || is_nacl_nonsfi) { On 2015/12/08 23:36:50, jln (slow on Chromium) wrote: > nit: it would be better if is_nacl_nonsfi would imply use_seccomp_bpf instead. > No big deal and no need to do that in this CL. OK, I'd rather do that as a separate change once we have tests in place. https://codereview.chromium.org/1497543006/diff/140001/sandbox/linux/BUILD.gn... sandbox/linux/BUILD.gn:84: sources -= [ On 2015/12/08 23:36:50, jln (slow on Chromium) wrote: > I'm a bit alergic to removing files, it makes things hard to understand. Do you > mind doing a conditional += above instead? Done.
phosek@chromium.org changed reviewers: + thakis@chromium.org
Nico, would it be possible to do OWNERS review for base/test and base/json?
components/nacl/loader/nonsfi LGTM. (Sorry, I was OOO yesterday).
phosek@chromium.org changed reviewers: + brettw@chromium.org
Brett, would it be possible to do a review for base/test and base/json?
don't you need to edit .gyp files too?
oh i see, it builds fine in gyp already. lgtm
sbc@chromium.org changed reviewers: + sbc@chromium.org
Is this ready to land now?
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by phosek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcgrathr@chromium.org, dpranke@chromium.org, jln@chromium.org, thakis@chromium.org, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/1497543006/#ps200001 (title: "Rebase")
The CQ bit was unchecked by phosek@chromium.org
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/1497543006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1497543006/200001
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: + bradnelson@chromium.org
I seem to be missing an lgtm from Brad; Brad could you please take a look?
The CQ bit was checked by bradnelson@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1497543006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1497543006/200001
Message was sent while issue was closed.
Description was changed from ========== GN: Build nacl_helper_nonsfi unit tests The DoSocketpair function in nonsfi_sandbox_unittest.cc is unused which causes a build error due to -Wunused-function which is set by default in GN hence removing the function. BUG=564877 ========== to ========== GN: Build nacl_helper_nonsfi unit tests The DoSocketpair function in nonsfi_sandbox_unittest.cc is unused which causes a build error due to -Wunused-function which is set by default in GN hence removing the function. BUG=564877 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== GN: Build nacl_helper_nonsfi unit tests The DoSocketpair function in nonsfi_sandbox_unittest.cc is unused which causes a build error due to -Wunused-function which is set by default in GN hence removing the function. BUG=564877 ========== to ========== GN: Build nacl_helper_nonsfi unit tests The DoSocketpair function in nonsfi_sandbox_unittest.cc is unused which causes a build error due to -Wunused-function which is set by default in GN hence removing the function. BUG=564877 Committed: https://crrev.com/ca8571a9777c5207d61dc28c323779e767b99bad Cr-Commit-Position: refs/heads/master@{#365836} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ca8571a9777c5207d61dc28c323779e767b99bad Cr-Commit-Position: refs/heads/master@{#365836} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
