Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
294593005:
Introduce create_nonsfi_test_nmf.py to simplify nacl_test_data.gyp (Closed)
|
Created:
6 years, 7 months ago by hidehiko Modified:
6 years, 7 months ago CC:
chromium-reviews, hamaji, Junichi Uekawa, mazda, teravest Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce create_nonsfi_test_nmf.py to simplify nacl_test_data.gyp
This CL is clean up of nacl_test_data.gyp file by supporting
nonsfi testing in ppapi_nacl_test_common.gypi.
For that purpose, this CL introduces a simple script
create_nonsfi_test_nmf.py, to generate .nmf files based on
gyp configurations.
TEST=Ran browser_tests --gtest_filter=NaClBrowserTest* locally, and ran trybots with --clobber.
BUG=368949
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272295
Patch Set 1 #
Total comments: 33
Patch Set 2 : Rebase #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 13 (0 generated)
Mark, Dave, could you take a look? Thank you for your review in advance, - hidehiko https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... File chrome/test/data/nacl/nacl_test_data.gyp (right): https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... chrome/test/data/nacl/nacl_test_data.gyp:640: # to build newlib linked nexes. FYI: generating unnecessary nexe files is what other tests do, so it should work.
LGTM with some changes... https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... File chrome/test/data/nacl/nacl_test_data.gyp (right): https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... chrome/test/data/nacl/nacl_test_data.gyp:41: # By this flag, test_files are copied into nonsfi directory, too. Maybe "This flag causes test_files to be copied..."? https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... chrome/test/data/nacl/nacl_test_data.gyp:636: # architecture, such as x86-32 (based on enable_XXX variables). "architectures" plural https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... chrome/test/data/nacl/nacl_test_data.gyp:656: 'nonsfi_destination_dir': '<(PRODUCT_DIR)/>(nexe_destination_dir)/nonsfi', Shouldn't this go away too? https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... chrome/test/data/nacl/nacl_test_data.gyp:657: # Workaround because generate_nmf doesn't work yet for NonSFI, Remove this comment? https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... File ppapi/ppapi_nacl_test_common.gypi (right): https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:69: ['test_files!=[] and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', { Can you assume that "enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1" is true, please? See comment below. https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:193: ['generate_nmf==1 and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', { I don't understand the reason for changing this. In what case would "enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1" be false? I think it would be better to assume that's always true. That would reduce the number of places in which we have to list all supported architectures. https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:211: ['generate_nmf==1 and build_pnacl_newlib==1 and disable_pnacl==0 and enable_x86_32_nonsfi', { Should this have "enable_x86_32_nonsfi==1" (add "==1") for consistency? https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:215: # If we support arm, we should split the dependency to Nit: "If we add support for ARM"? "dependency on" rather than "dependency to"? https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... File ppapi/tests/create_nonsfi_test_nmf.py (right): https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:1: #!/usr/bin/env python Can you remove "nonsfi" from the file's name, perhaps, e.g. "create_test_nmf.py"? It would be good to see the NMF generation become more consistent, later on. In later changes, we could use this to generate NMFs for all cases in which we generate statically-linked nexes (using either PNaCl or GCC toolchains), and it would be easier if we didn't have to rename this file. :-) https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:6: """Simple tool to generate NMF file from given arguments for nonsfi.""" Can you add a comment saying why this tool exists, comparing it to create_nmf.py in the SDK? How is it similar and how does it differ? https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:24: '--program', metavar='FILE', help='Main program file') Maybe s/file/nexe/ to be more specific? https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:30: ' esection of the .nmf')) Typo: "section" https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:54: # Note use path as is, unlike program path. Add colon -- "Note:"?
Thank you for review. PTAL. https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... File chrome/test/data/nacl/nacl_test_data.gyp (right): https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... chrome/test/data/nacl/nacl_test_data.gyp:41: # By this flag, test_files are copied into nonsfi directory, too. On 2014/05/19 18:55:29, Mark Seaborn wrote: > Maybe "This flag causes test_files to be copied..."? Done. https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... chrome/test/data/nacl/nacl_test_data.gyp:636: # architecture, such as x86-32 (based on enable_XXX variables). On 2014/05/19 18:55:29, Mark Seaborn wrote: > "architectures" plural Done. https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... chrome/test/data/nacl/nacl_test_data.gyp:656: 'nonsfi_destination_dir': '<(PRODUCT_DIR)/>(nexe_destination_dir)/nonsfi', On 2014/05/19 18:55:29, Mark Seaborn wrote: > Shouldn't this go away too? Good catch. Done. https://codereview.chromium.org/294593005/diff/1/chrome/test/data/nacl/nacl_t... chrome/test/data/nacl/nacl_test_data.gyp:657: # Workaround because generate_nmf doesn't work yet for NonSFI, On 2014/05/19 18:55:29, Mark Seaborn wrote: > Remove this comment? Done. https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... File ppapi/ppapi_nacl_test_common.gypi (right): https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:69: ['test_files!=[] and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', { On 2014/05/19 18:55:29, Mark Seaborn wrote: > Can you assume that "enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or > enable_mips==1" is true, please? > > See comment below. Acknowledged. https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:193: ['generate_nmf==1 and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', { On 2014/05/19 18:55:29, Mark Seaborn wrote: > I don't understand the reason for changing this. > > In what case would "enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or > enable_mips==1" be false? > > I think it would be better to assume that's always true. That would reduce the > number of places in which we have to list all supported architectures. Conceptually, we should, for irt_manifest_file, if possible. Currently build_pnacl_newlib is used for both; 1) building a pexe binary 2) building a nexe binary for x86-32-nonsfi These are conceptually independent, but nonsfi nexe binary is translated from pexe binary so the flag is needed, as you know. Due to some reason, we may want to enable only 2). Current irt_manifest_file is an example, I think. (Indeed, ideally, we probably want to enable both 1) and 2) for irt_manifest_file, though) It may not actually cause a problem, but building unnecessary targets looked confusing to me, so I just tried to get rid of it. Currently we have four categories for tests, which are newlib, glibc, pnacl and nonsfi. Before adding nonsfi, build_newlib, build_glibc, build_pnacl_newlib was one-to-one-onto mapping for them. However, nonsfi is a bit tricky as it is "build_pnacl_newlib = 1 && enable_x86_32_nonsfi". So probably, an alternative approach would be introducing new variables, e.g.; enable_nacl_newlib_test enable_nacl_glibc_test enable_nacl_pnacl_test enable_nacl_nonsfi_test WDYT? Please let me keep this as is in this patch set. I'll revisit here, upon your reply. https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:211: ['generate_nmf==1 and build_pnacl_newlib==1 and disable_pnacl==0 and enable_x86_32_nonsfi', { On 2014/05/19 18:55:29, Mark Seaborn wrote: > Should this have "enable_x86_32_nonsfi==1" (add "==1") for consistency? Done. https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:215: # If we support arm, we should split the dependency to On 2014/05/19 18:55:29, Mark Seaborn wrote: > Nit: "If we add support for ARM"? "dependency on" rather than "dependency to"? Done. https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... File ppapi/tests/create_nonsfi_test_nmf.py (right): https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:1: #!/usr/bin/env python On 2014/05/19 18:55:29, Mark Seaborn wrote: > Can you remove "nonsfi" from the file's name, perhaps, e.g. > "create_test_nmf.py"? > > It would be good to see the NMF generation become more consistent, later on. In > later changes, we could use this to generate NMFs for all cases in which we > generate statically-linked nexes (using either PNaCl or GCC toolchains), and it > would be easier if we didn't have to rename this file. :-) I'm not still very sure if this should be reused for generating other nmf files. This looks currently simple, but if we need to support either 1) multi-arch 2) .so linked nmf and 3) runnable-ld.so, the code would rapidly become gigantic, complicated, and looked just yet-another implementation of create_nmf.py. (Actually, I tried :-) IIUC, actually these are used for testing. For 1), as you said, Win uses it. 2) and 3) are necessary for glibc testing now. If we could replace all create_nmf.py, it would be an option. However, it is not the case. And, to be honest, using create_nmf.py sounds better approach, because it is closer to what users (developers) does. Moreover, create_nmf.py does more than nmf creation. E.g., it also stages the needed files. Do we want to use this for other nmf file generation, although it should be supported by create_nmf.py? I added "nonsfi" to the file name in the sense that; this script is used only for nonsfi because create_nmf.py does not support generating nmf files for nonsfi. WDYT? https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:6: """Simple tool to generate NMF file from given arguments for nonsfi.""" On 2014/05/19 18:55:29, Mark Seaborn wrote: > Can you add a comment saying why this tool exists, comparing it to create_nmf.py > in the SDK? How is it similar and how does it differ? Could you tell me the future plans as I asked above? I'll write more comments based on them. Currently, this is just a kind of lesser copy of create_nmf.py only for nonsfi. https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:24: '--program', metavar='FILE', help='Main program file') On 2014/05/19 18:55:29, Mark Seaborn wrote: > Maybe s/file/nexe/ to be more specific? Done. https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:30: ' esection of the .nmf')) On 2014/05/19 18:55:29, Mark Seaborn wrote: > Typo: "section" Done. https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:54: # Note use path as is, unlike program path. On 2014/05/19 18:55:29, Mark Seaborn wrote: > Add colon -- "Note:"? Done.
lgtm https://chromiumcodereview.appspot.com/294593005/diff/60001/ppapi/ppapi_nacl_... File ppapi/ppapi_nacl_test_common.gypi (right): https://chromiumcodereview.appspot.com/294593005/diff/60001/ppapi/ppapi_nacl_... ppapi/ppapi_nacl_test_common.gypi:69: ['test_files!=[] and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', { These conditionals are getting pretty big and hard-to-read. Is there any way to make a variable and conditional that would wrap up all the architectures? Something like: https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...
LGTM. Two more things: * Have you tested locally that the NaCl tests work after a clobber build (after deleting the NMF files, at least)? The trybots/CQ probably wouldn't catch it if you accidentally stopped generating a required NMF. * Does this change allow any of the following to be removed? - ppapi/tests/ppapi_nacl_tests_pnacl_nonsfi.nmf - chrome/test/data/nacl/irt_exception/irt_exception_test.nmf - "nonsfi" section in chrome/test/data/nacl/manifest_file/irt_manifest_file.nmf https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... File ppapi/ppapi_nacl_test_common.gypi (right): https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:193: ['generate_nmf==1 and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', { On 2014/05/20 05:51:47, hidehiko wrote: > On 2014/05/19 18:55:29, Mark Seaborn wrote: > > I don't understand the reason for changing this. > > > > In what case would "enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or > > enable_mips==1" be false? > > > > I think it would be better to assume that's always true. That would reduce > > the number of places in which we have to list all supported architectures. > ... > It may not actually cause a problem, but building unnecessary targets looked > confusing to me, so I just tried to get rid of it. OK, let's leave this as is. My main problem with this extra condition was that it's not very clear what it's for -- whether it is: * just an optimisation (avoiding building something we don't need); * needed for correctness (maybe the build fails if we try this when no inputs are present). I'm not sure whether it's worth commenting this in the gyp file. > Currently we have four categories for tests, which are newlib, glibc, pnacl and > nonsfi. Before adding nonsfi, build_newlib, build_glibc, build_pnacl_newlib was > one-to-one-onto mapping for them. However, nonsfi is a bit tricky as it is > "build_pnacl_newlib = 1 && enable_x86_32_nonsfi". So probably, an alternative > approach would be introducing new variables, e.g.; > enable_nacl_newlib_test > enable_nacl_glibc_test > enable_nacl_pnacl_test > enable_nacl_nonsfi_test If we need labels like this, they would be clearer as: enable_pnacl_pexe_test enable_pnacl_sfi_nexe_test enable_pnacl_nonsfi_nexe_test So that it's clearer whether the NMFs list pexes or nexes. But this is just an aside which doesn't need to affect your current change. https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... File ppapi/tests/create_nonsfi_test_nmf.py (right): https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:1: #!/usr/bin/env python On 2014/05/20 05:51:47, hidehiko wrote: > On 2014/05/19 18:55:29, Mark Seaborn wrote: > > Can you remove "nonsfi" from the file's name, perhaps, e.g. > > "create_test_nmf.py"? > > > > It would be good to see the NMF generation become more consistent, later on. > In > > later changes, we could use this to generate NMFs for all cases in which we > > generate statically-linked nexes (using either PNaCl or GCC toolchains), and > it > > would be easier if we didn't have to rename this file. :-) > > I'm not still very sure if this should be reused for generating other nmf files. > This looks currently simple, but if we need to support either 1) multi-arch 2) > .so linked nmf and 3) runnable-ld.so, the code would rapidly become gigantic, > complicated, and looked just yet-another implementation of create_nmf.py. > (Actually, I tried :-) OK, let's not get bogged down in this review with discussing how subsequent changes could be done. :-) We can talk about that after. Sticking with the name "create_nonsfi_test_nmf.py" is fine for this change. The change is fine since it does make the Gyp logic more regular. https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:6: """Simple tool to generate NMF file from given arguments for nonsfi.""" On 2014/05/20 05:51:47, hidehiko wrote: > On 2014/05/19 18:55:29, Mark Seaborn wrote: > > Can you add a comment saying why this tool exists, comparing it to > > create_nmf.py in the SDK? How is it similar and how does it differ? > > Could you tell me the future plans as I asked above? > I'll write more comments based on them. Currently, this is just a kind of lesser > copy of create_nmf.py only for nonsfi. The comment doesn't need to describe future plans, just the current state. How about something like: This tool is similar to native_client_sdk/src/tools/create_nmf.py. create_nmf.py handles most cases, with the exception of Non-SFI nexes. create_nmf.py tries to auto-detect nexe and pexe types based on their contents, but this does not work for Non-SFI nexes (which don't have a marker to distinguish them from SFI nexes). create_nonsfi_test_nmf.py is a simpler tool which essentially just reformats its command line arguments, converting them to JSON.
Thank you for review. Sorry, but it turned out an issue, so I changed a bit. Could you kindly take another look? > * Have you tested locally that the NaCl tests work after a clobber build (after > deleting the NMF files, at least)? The trybots/CQ probably wouldn't catch it if > you accidentally stopped generating a required NMF. Good catch. I sometimes build after removing out/ directory locally, but only for nonsfi. And, it turned out that the PS3 actually breaks clobber build for pnacl test. The problem was caused by pnacl's condition, so I just reverted it as you pointed at first. (Now I'm running trybots with --clobber option). > * Does this change allow any of the following to be removed? > > - ppapi/tests/ppapi_nacl_tests_pnacl_nonsfi.nmf > - chrome/test/data/nacl/irt_exception/irt_exception_test.nmf > - "nonsfi" section in chrome/test/data/nacl/manifest_file/irt_manifest_file.nmf I removed irt_*.nmf. ppapi_nacl_tests_pnacl_nonsfi.nmf is used differently so let me keep it as is for now. I'll address it in a separate CL (after this CL is landed). https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... File ppapi/ppapi_nacl_test_common.gypi (right): https://codereview.chromium.org/294593005/diff/1/ppapi/ppapi_nacl_test_common... ppapi/ppapi_nacl_test_common.gypi:193: ['generate_nmf==1 and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', { On 2014/05/21 00:08:39, Mark Seaborn wrote: > On 2014/05/20 05:51:47, hidehiko wrote: > > On 2014/05/19 18:55:29, Mark Seaborn wrote: > > > I don't understand the reason for changing this. > > > > > > In what case would "enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or > > > enable_mips==1" be false? > > > > > > I think it would be better to assume that's always true. That would reduce > > > the number of places in which we have to list all supported architectures. > > > ... > > It may not actually cause a problem, but building unnecessary targets looked > > confusing to me, so I just tried to get rid of it. > > OK, let's leave this as is. > > My main problem with this extra condition was that it's not very clear what it's > for -- whether it is: > > * just an optimisation (avoiding building something we don't need); > * needed for correctness (maybe the build fails if we try this when no inputs > are present). > > I'm not sure whether it's worth commenting this in the gyp file. > > > > Currently we have four categories for tests, which are newlib, glibc, pnacl > and > > nonsfi. Before adding nonsfi, build_newlib, build_glibc, build_pnacl_newlib > was > > one-to-one-onto mapping for them. However, nonsfi is a bit tricky as it is > > "build_pnacl_newlib = 1 && enable_x86_32_nonsfi". So probably, an alternative > > approach would be introducing new variables, e.g.; > > enable_nacl_newlib_test > > enable_nacl_glibc_test > > enable_nacl_pnacl_test > > enable_nacl_nonsfi_test > > If we need labels like this, they would be clearer as: > > enable_pnacl_pexe_test > enable_pnacl_sfi_nexe_test > enable_pnacl_nonsfi_nexe_test > > So that it's clearer whether the NMFs list pexes or nexes. But this is just an > aside which doesn't need to affect your current change. Seems like we want to control sfi glibc and sfi newlib test building separately? Anyway, I'll try to work on it as another clean up in a separate CL. https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... File ppapi/tests/create_nonsfi_test_nmf.py (right): https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:1: #!/usr/bin/env python On 2014/05/21 00:08:39, Mark Seaborn wrote: > On 2014/05/20 05:51:47, hidehiko wrote: > > On 2014/05/19 18:55:29, Mark Seaborn wrote: > > > Can you remove "nonsfi" from the file's name, perhaps, e.g. > > > "create_test_nmf.py"? > > > > > > It would be good to see the NMF generation become more consistent, later on. > > > In > > > later changes, we could use this to generate NMFs for all cases in which we > > > generate statically-linked nexes (using either PNaCl or GCC toolchains), and > > it > > > would be easier if we didn't have to rename this file. :-) > > > > I'm not still very sure if this should be reused for generating other nmf > files. > > This looks currently simple, but if we need to support either 1) multi-arch 2) > > .so linked nmf and 3) runnable-ld.so, the code would rapidly become gigantic, > > complicated, and looked just yet-another implementation of create_nmf.py. > > (Actually, I tried :-) > > OK, let's not get bogged down in this review with discussing how subsequent > changes could be done. :-) We can talk about that after. Sticking with the > name "create_nonsfi_test_nmf.py" is fine for this change. The change is fine > since it does make the Gyp logic more regular. Acknowledged. Thanks for reply. https://codereview.chromium.org/294593005/diff/1/ppapi/tests/create_nonsfi_te... ppapi/tests/create_nonsfi_test_nmf.py:6: """Simple tool to generate NMF file from given arguments for nonsfi.""" On 2014/05/21 00:08:39, Mark Seaborn wrote: > On 2014/05/20 05:51:47, hidehiko wrote: > > On 2014/05/19 18:55:29, Mark Seaborn wrote: > > > Can you add a comment saying why this tool exists, comparing it to > > > create_nmf.py in the SDK? How is it similar and how does it differ? > > > > Could you tell me the future plans as I asked above? > > I'll write more comments based on them. Currently, this is just a kind of > lesser > > copy of create_nmf.py only for nonsfi. > > The comment doesn't need to describe future plans, just the current state. > > How about something like: > > This tool is similar to native_client_sdk/src/tools/create_nmf.py. > create_nmf.py handles most cases, with the exception of Non-SFI nexes. > create_nmf.py tries to auto-detect nexe and pexe types based on their contents, > but this does not work for Non-SFI nexes (which don't have a marker to > distinguish them from SFI nexes). > > create_nonsfi_test_nmf.py is a simpler tool which essentially just reformats its > command line arguments, converting them to JSON. Done. Thank you for suggestion. How about this? https://codereview.chromium.org/294593005/diff/60001/ppapi/ppapi_nacl_test_co... File ppapi/ppapi_nacl_test_common.gypi (right): https://codereview.chromium.org/294593005/diff/60001/ppapi/ppapi_nacl_test_co... ppapi/ppapi_nacl_test_common.gypi:69: ['test_files!=[] and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', { On 2014/05/20 19:45:35, dmichael wrote: > These conditionals are getting pretty big and hard-to-read. > > Is there any way to make a variable and conditional that would wrap up all the > architectures? Something like: > https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... Due to evaluation order, it seems to cause a gyp error. I'm thinking that introducing a new variable makes the gyp more readable as I replied to Mark. Please let me try to work on it in a separate CL.
LGTM
lgtm
The CQ bit was checked by hidehiko@chromium.org
The CQ bit was unchecked by hidehiko@chromium.org
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/294593005/90001
Message was sent while issue was closed.
Change committed as 272295 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
