Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2884353004:
fuchsia: base_unittests (mostly) compiling (Closed)
|
Created:
3 years, 7 months ago by scottmg Modified:
3 years, 7 months ago Reviewers:
Nico CC:
chromium-reviews, sadrul, vmpstr+watch_chromium.org, wfh+watch_chromium.org, dominicc+watchlist_chromium.org, ail_google.com, tracing+reviews_chromium.org, danakj+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionfuchsia: base_unittests (mostly) compiling
Things that are compiled out of base at the moment (without replacements
implemented in this CL): message loop, process control, signal handling
in test runner, and backtraces, all of which will need to be implemented
in Fuchsia-specific ways in subsequent patches, rather than using the
Linux-y/Posix-y ways.
Two functions in trace_event are also stubbed for now, not for any good
reason, just because I'm not sure how to implement them yet, and this
gets things compiling.
BUG=706592
Review-Url: https://codereview.chromium.org/2884353004
Cr-Commit-Position: refs/heads/master@{#472983}
Committed: https://chromium.googlesource.com/chromium/src/+/6ea9ff3e27ed759a26158a6e71fd3d9ea7789aeb
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #
Total comments: 14
Patch Set 3 : fixes #Patch Set 4 : rebase #
Messages
Total messages: 37 (25 generated)
Description was changed from ========== fuchsia: base_unittests compiling (Not linking yet.) BUG=706592 ========== to ========== fuchsia: base_unittests mostly compiling Three main things compiled out: - things that require file descriptor watching - process control - message loop details File descriptors will likely be implemented differently than This gets all of the .cc BUG=706592 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== fuchsia: base_unittests mostly compiling Three main things compiled out: - things that require file descriptor watching - process control - message loop details File descriptors will likely be implemented differently than This gets all of the .cc BUG=706592 ========== to ========== fuchsia: base_unittests (mostly) compiling Two things are compiled out: message loop which will be implemented in BUG=706592 ==========
Description was changed from ========== fuchsia: base_unittests (mostly) compiling Two things are compiled out: message loop which will be implemented in BUG=706592 ========== to ========== fuchsia: base_unittests (mostly) compiling Three things are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, and backtraces, all of which will be implemented in Fuchsia-specific ways in subsequent patches, rather than in Linux-y/Posix-y ways. BUG=706592 ==========
Description was changed from ========== fuchsia: base_unittests (mostly) compiling Three things are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, and backtraces, all of which will be implemented in Fuchsia-specific ways in subsequent patches, rather than in Linux-y/Posix-y ways. BUG=706592 ========== to ========== fuchsia: base_unittests (mostly) compiling Things are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, signal handling in test runner, and backtraces, all of which will need to be implemented in Fuchsia-specific ways in subsequent patches, rather than using the Linux-y/Posix-y ways. BUG=706592 ==========
Patchset #2 (id:40001) has been deleted
Description was changed from ========== fuchsia: base_unittests (mostly) compiling Things are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, signal handling in test runner, and backtraces, all of which will need to be implemented in Fuchsia-specific ways in subsequent patches, rather than using the Linux-y/Posix-y ways. BUG=706592 ========== to ========== fuchsia: base_unittests (mostly) compiling Things are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, signal handling in test runner, and backtraces, all of which will need to be implemented in Fuchsia-specific ways in subsequent patches, rather than using the Linux-y/Posix-y ways. Two functions in trace_event are also stubbed for now, not for any good reason, just because I'm not sure how to implement them yet, and this gets things compiling. BUG=706592 ==========
Description was changed from ========== fuchsia: base_unittests (mostly) compiling Things are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, signal handling in test runner, and backtraces, all of which will need to be implemented in Fuchsia-specific ways in subsequent patches, rather than using the Linux-y/Posix-y ways. Two functions in trace_event are also stubbed for now, not for any good reason, just because I'm not sure how to implement them yet, and this gets things compiling. BUG=706592 ========== to ========== fuchsia: base_unittests (mostly) compiling Things are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, signal handling in test runner, and backtraces, all of which will need to be implemented in Fuchsia-specific ways in subsequent patches, rather than using the Linux-y/Posix-y ways. Two functions in trace_event are also stubbed for now, not for any good reason, just because I'm not sure how to implement them yet, and this gets things compiling. BUG=706592 ==========
scottmg@chromium.org changed reviewers: + thakis@chromium.org
The CQ bit was checked by scottmg@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...
https://codereview.chromium.org/2884353004/diff/1/base/files/file_util_posix.cc File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2884353004/diff/1/base/files/file_util_posix.... base/files/file_util_posix.cc:59: #include <sys/errno.h> ../../third_party/fuchsia-sdk/sysroot/x86_64-fuchsia/include/sys/errno.h:1:2: error: redirecting incorrect #include <sys/errno.h> to <errno.h> [-Werror,-W#warnings] #warning redirecting incorrect #include <sys/errno.h> to <errno.h> https://codereview.chromium.org/2884353004/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2884353004/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1094: if ((is_linux && target_cpu == "x86") || is_fuchsia) { ../../third_party/icu/source/common/ustr_wcs.cpp:345:13: error: 'register' storage class specifier is deprecated and incompatible with C++1z [-Werror,-Wdeprecated-register] register int32_t nulLen = 0; ^~~~~~~~~
Description was changed from ========== fuchsia: base_unittests (mostly) compiling Things are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, signal handling in test runner, and backtraces, all of which will need to be implemented in Fuchsia-specific ways in subsequent patches, rather than using the Linux-y/Posix-y ways. Two functions in trace_event are also stubbed for now, not for any good reason, just because I'm not sure how to implement them yet, and this gets things compiling. BUG=706592 ========== to ========== fuchsia: base_unittests (mostly) compiling Things that are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, signal handling in test runner, and backtraces, all of which will need to be implemented in Fuchsia-specific ways in subsequent patches, rather than using the Linux-y/Posix-y ways. Two functions in trace_event are also stubbed for now, not for any good reason, just because I'm not sure how to implement them yet, and this gets things compiling. BUG=706592 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
That's surprisingly un-gross so far! https://codereview.chromium.org/2884353004/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2884353004/diff/60001/base/BUILD.gn#newcode1233 base/BUILD.gn:1233: if (is_fuchsia) { You are in a maze of twisty passages, all alike. (this is as designed per comment on top of this file, but this build file is pretty confusing. Still less confusing that chrome/test/BUILD.gn. I don't have any good suggestions.) https://codereview.chromium.org/2884353004/diff/60001/base/files/file_util_po... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2884353004/diff/60001/base/files/file_util_po... base/files/file_util_posix.cc:59: #include <sys/errno.h> Doesn't the warning just let you know that this should be <errno.h>, not <sys/errno.h>? Have you tried just deleting "sys/"? https://codereview.chromium.org/2884353004/diff/60001/base/message_loop/messa... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/2884353004/diff/60001/base/message_loop/messa... base/message_loop/message_loop.cc:172: // Currently NaCl doesn't have a UI MessageLoop. Probably want to update this comment. If the expectation is that you'll have an UI eventually, also add a TODO https://codereview.chromium.org/2884353004/diff/60001/base/message_loop/messa... base/message_loop/message_loop.cc:647: #endif So this function is just not implemented on fuchsia and will cause a linker error if used? (Seems fine.) https://codereview.chromium.org/2884353004/diff/60001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2884353004/diff/60001/base/test/launcher/test... base/test/launcher/test_launcher.cc:527: #if defined(OS_POSIX) && !defined(OS_FUCHSIA) Do you expect to never need this? If so, why not? (Add comment.) If you think we should add it later, add TODO. https://codereview.chromium.org/2884353004/diff/60001/base/trace_event/malloc... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/2884353004/diff/60001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.cc:246: // TODO(scottmg): Port, see https://crbug.com/706592. Please use TODO(fuchsia) for all these, for greppability https://codereview.chromium.org/2884353004/diff/60001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2884353004/diff/60001/build/config/compiler/B... build/config/compiler/BUILD.gn:1094: if ((is_linux && target_cpu == "x86") || is_fuchsia) { 1.) If this happens only in ICU, we should suppress it only there, not globally. 2.) Why does this happen for ICU only for fuchsia?
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by scottmg@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 #3 (id:100001) has been deleted
The CQ bit was checked by scottmg@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! https://codereview.chromium.org/2884353004/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2884353004/diff/60001/base/BUILD.gn#newcode1233 base/BUILD.gn:1233: if (is_fuchsia) { On 2017/05/17 15:52:53, Nico wrote: > You are in a maze of twisty passages, all alike. > > (this is as designed per comment on top of this file, but this build file is > pretty confusing. Still less confusing that chrome/test/BUILD.gn. I don't have > any good suggestions.) Yeah, it's a little confusing. I thought about adding a uses_libevent bool for those ones, but I'm not sure that would really clear things up. For the process ones, I think they're definitely going to have to be separate implementations, but it would also wouldn't make much sense for Fuchsia to not get the rest of the _posix files. So... shrug. There'll be about 5-10 +='d _fuchsia's here eventually also. https://codereview.chromium.org/2884353004/diff/60001/base/files/file_util_po... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2884353004/diff/60001/base/files/file_util_po... base/files/file_util_posix.cc:59: #include <sys/errno.h> On 2017/05/17 15:52:53, Nico wrote: > Doesn't the warning just let you know that this should be <errno.h>, not > <sys/errno.h>? Have you tried just deleting "sys/"? Ah, I assumed this was a Fuchsia-specific thing, but I just check a Linux and Android sys/errno.h and they just do #include <errno.h> without the #warning. So I just deleted this. (We already #include <errno.h> earlier in this file. https://codereview.chromium.org/2884353004/diff/60001/base/message_loop/messa... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/2884353004/diff/60001/base/message_loop/messa... base/message_loop/message_loop.cc:172: // Currently NaCl doesn't have a UI MessageLoop. On 2017/05/17 15:52:53, Nico wrote: > Probably want to update this comment. If the expectation is that you'll have an > UI eventually, also add a TODO Done. https://codereview.chromium.org/2884353004/diff/60001/base/message_loop/messa... base/message_loop/message_loop.cc:647: #endif On 2017/05/17 15:52:54, Nico wrote: > So this function is just not implemented on fuchsia and will cause a linker > error if used? (Seems fine.) Yes, just a link failure right now. I think it'll have to be a Fuchsia syscall to do an fd watch, but depends on MessageLoop which I'm not clear on the implementation for yet. https://codereview.chromium.org/2884353004/diff/60001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2884353004/diff/60001/base/test/launcher/test... base/test/launcher/test_launcher.cc:527: #if defined(OS_POSIX) && !defined(OS_FUCHSIA) On 2017/05/17 15:52:54, Nico wrote: > Do you expect to never need this? If so, why not? (Add comment.) If you think we > should add it later, add TODO. Done. https://codereview.chromium.org/2884353004/diff/60001/base/trace_event/malloc... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/2884353004/diff/60001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.cc:246: // TODO(scottmg): Port, see https://crbug.com/706592. On 2017/05/17 15:52:54, Nico wrote: > Please use TODO(fuchsia) for all these, for greppability Done. https://codereview.chromium.org/2884353004/diff/60001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2884353004/diff/60001/build/config/compiler/B... build/config/compiler/BUILD.gn:1094: if ((is_linux && target_cpu == "x86") || is_fuchsia) { On 2017/05/17 15:52:54, Nico wrote: > 1.) If this happens only in ICU, we should suppress it only there, not globally. > 2.) Why does this happen for ICU only for fuchsia? Hmm, #2 is a good question... here's the command lines https://gist.github.com/sgraham/c806b636d0d58df4f534d100960df103 Digging into the flags a bit (%s/ /^M, %!sort, %!uniq, diff) I get this: [scottmg:~]$ diff linux.txt fuchsia.txt 2,3c2,3 < --sysroot=../../build/linux/debian_jessie_amd64-sysroot < -B../../third_party/binutils/Linux_x64/Release/bin --- > --sysroot=../../third_party/fuchsia-sdk/sysroot/x86_64-fuchsia > --target=x86_64-fuchsia 12a13 > -DNO_TCMALLOC 15a17 > -DSYSROOT_VERSION=c831fd41032acfe5ce360cd0459511a0ec3338eb 18,24d19 < -DUSE_AURA=1 < -DUSE_CAIRO=1 < -DUSE_GLIB=1 < -DUSE_NSS_CERTS=1 < -DUSE_PANGO=1 < -DUSE_UDEV < -DUSE_X11=1 52d46 < -Wno-deprecated 84d77 < -load 90,91c83 < -pthread < -std=gnu++11 --- > -std=c++11 94d85 < ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so 96d86 < check-ipc I was hoping it was -std=gnu++11 vs c++11 but it's actually that I didn't turn on all of the -Wno-deprecated https://cs.chromium.org/chromium/src/build/config/compiler/BUILD.gn?rcl=5f7ac... "for hash_map".
Hm, I think we want to get rid of -Wno-deprecated on Linux globally eventually. (We manage to build without it on Mac and Win.) So I still think this should move to ICU for fuchsia, and shouldn't be toggled globally. On May 17, 2017 12:52 PM, <scottmg@chromium.org> wrote: > Thanks! > > > https://codereview.chromium.org/2884353004/diff/60001/base/BUILD.gn > File base/BUILD.gn (right): > > https://codereview.chromium.org/2884353004/diff/60001/ > base/BUILD.gn#newcode1233 > base/BUILD.gn:1233: if (is_fuchsia) { > On 2017/05/17 15:52:53, Nico wrote: > > You are in a maze of twisty passages, all alike. > > > > (this is as designed per comment on top of this file, but this build > file is > > pretty confusing. Still less confusing that chrome/test/BUILD.gn. I > don't have > > any good suggestions.) > > Yeah, it's a little confusing. I thought about adding a uses_libevent > bool for those ones, but I'm not sure that would really clear things up. > > For the process ones, I think they're definitely going to have to be > separate implementations, but it would also wouldn't make much sense for > Fuchsia to not get the rest of the _posix files. So... shrug. There'll > be about 5-10 +='d _fuchsia's here eventually also. > > https://codereview.chromium.org/2884353004/diff/60001/ > base/files/file_util_posix.cc > File base/files/file_util_posix.cc (right): > > https://codereview.chromium.org/2884353004/diff/60001/ > base/files/file_util_posix.cc#newcode59 > base/files/file_util_posix.cc:59: #include <sys/errno.h> > On 2017/05/17 15:52:53, Nico wrote: > > Doesn't the warning just let you know that this should be <errno.h>, > not > > <sys/errno.h>? Have you tried just deleting "sys/"? > > Ah, I assumed this was a Fuchsia-specific thing, but I just check a > Linux and Android sys/errno.h and they just do #include <errno.h> > without the #warning. So I just deleted this. (We already #include > <errno.h> earlier in this file. > > https://codereview.chromium.org/2884353004/diff/60001/ > base/message_loop/message_loop.cc > File base/message_loop/message_loop.cc (right): > > https://codereview.chromium.org/2884353004/diff/60001/ > base/message_loop/message_loop.cc#newcode172 > base/message_loop/message_loop.cc:172: // Currently NaCl doesn't have a > UI MessageLoop. > On 2017/05/17 15:52:53, Nico wrote: > > Probably want to update this comment. If the expectation is that > you'll have an > > UI eventually, also add a TODO > > Done. > > https://codereview.chromium.org/2884353004/diff/60001/ > base/message_loop/message_loop.cc#newcode647 > base/message_loop/message_loop.cc:647: #endif > On 2017/05/17 15:52:54, Nico wrote: > > So this function is just not implemented on fuchsia and will cause a > linker > > error if used? (Seems fine.) > > Yes, just a link failure right now. I think it'll have to be a Fuchsia > syscall to do an fd watch, but depends on MessageLoop which I'm not > clear on the implementation for yet. > > https://codereview.chromium.org/2884353004/diff/60001/ > base/test/launcher/test_launcher.cc > File base/test/launcher/test_launcher.cc (right): > > https://codereview.chromium.org/2884353004/diff/60001/ > base/test/launcher/test_launcher.cc#newcode527 > base/test/launcher/test_launcher.cc:527: #if defined(OS_POSIX) && > !defined(OS_FUCHSIA) > On 2017/05/17 15:52:54, Nico wrote: > > Do you expect to never need this? If so, why not? (Add comment.) If > you think we > > should add it later, add TODO. > > Done. > > https://codereview.chromium.org/2884353004/diff/60001/ > base/trace_event/malloc_dump_provider.cc > File base/trace_event/malloc_dump_provider.cc (right): > > https://codereview.chromium.org/2884353004/diff/60001/ > base/trace_event/malloc_dump_provider.cc#newcode246 > base/trace_event/malloc_dump_provider.cc:246: // TODO(scottmg): Port, > see https://crbug.com/706592. > On 2017/05/17 15:52:54, Nico wrote: > > Please use TODO(fuchsia) for all these, for greppability > > Done. > > https://codereview.chromium.org/2884353004/diff/60001/ > build/config/compiler/BUILD.gn > File build/config/compiler/BUILD.gn (right): > > https://codereview.chromium.org/2884353004/diff/60001/ > build/config/compiler/BUILD.gn#newcode1094 > build/config/compiler/BUILD.gn:1094: if ((is_linux && target_cpu == > "x86") || is_fuchsia) { > On 2017/05/17 15:52:54, Nico wrote: > > 1.) If this happens only in ICU, we should suppress it only there, not > globally. > > 2.) Why does this happen for ICU only for fuchsia? > > Hmm, #2 is a good question... here's the command lines > https://gist.github.com/sgraham/c806b636d0d58df4f534d100960df103 > > Digging into the flags a bit (%s/ /^M, %!sort, %!uniq, diff) I get this: > > [scottmg:~]$ diff linux.txt fuchsia.txt > 2,3c2,3 > < --sysroot=../../build/linux/debian_jessie_amd64-sysroot > < -B../../third_party/binutils/Linux_x64/Release/bin > --- > > --sysroot=../../third_party/fuchsia-sdk/sysroot/x86_64-fuchsia > > --target=x86_64-fuchsia > 12a13 > > -DNO_TCMALLOC > 15a17 > > -DSYSROOT_VERSION=c831fd41032acfe5ce360cd0459511a0ec3338eb > 18,24d19 > < -DUSE_AURA=1 > < -DUSE_CAIRO=1 > < -DUSE_GLIB=1 > < -DUSE_NSS_CERTS=1 > < -DUSE_PANGO=1 > < -DUSE_UDEV > < -DUSE_X11=1 > 52d46 > < -Wno-deprecated > 84d77 > < -load > 90,91c83 > < -pthread > < -std=gnu++11 > --- > > -std=c++11 > 94d85 > < > ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so > 96d86 > < check-ipc > > I was hoping it was -std=gnu++11 vs c++11 but it's actually that I > didn't turn on all of the -Wno-deprecated > https://cs.chromium.org/chromium/src/build/config/compiler/BUILD.gn?rcl= > 5f7ac3af3be63d35ecba7c6814c842f4d1c12da3&l=1211 > "for hash_map". > > https://codereview.chromium.org/2884353004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/17 18:09:34, Nico wrote: > Hm, I think we want to get rid of -Wno-deprecated on Linux globally > eventually. (We manage to build without it on Mac and Win.) So I still > think this should move to ICU for fuchsia, and shouldn't be toggled > globally. OK, lemme see in a precursor CL if I can localize it more for non-chromium_code for Linux first and then maybe this CL will not have to add a special if for Fuchsia. > > On May 17, 2017 12:52 PM, <mailto:scottmg@chromium.org> wrote: > > > Thanks! > > > > > > https://codereview.chromium.org/2884353004/diff/60001/base/BUILD.gn > > File base/BUILD.gn (right): > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/BUILD.gn#newcode1233 > > base/BUILD.gn:1233: if (is_fuchsia) { > > On 2017/05/17 15:52:53, Nico wrote: > > > You are in a maze of twisty passages, all alike. > > > > > > (this is as designed per comment on top of this file, but this build > > file is > > > pretty confusing. Still less confusing that chrome/test/BUILD.gn. I > > don't have > > > any good suggestions.) > > > > Yeah, it's a little confusing. I thought about adding a uses_libevent > > bool for those ones, but I'm not sure that would really clear things up. > > > > For the process ones, I think they're definitely going to have to be > > separate implementations, but it would also wouldn't make much sense for > > Fuchsia to not get the rest of the _posix files. So... shrug. There'll > > be about 5-10 +='d _fuchsia's here eventually also. > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/files/file_util_posix.cc > > File base/files/file_util_posix.cc (right): > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/files/file_util_posix.cc#newcode59 > > base/files/file_util_posix.cc:59: #include <sys/errno.h> > > On 2017/05/17 15:52:53, Nico wrote: > > > Doesn't the warning just let you know that this should be <errno.h>, > > not > > > <sys/errno.h>? Have you tried just deleting "sys/"? > > > > Ah, I assumed this was a Fuchsia-specific thing, but I just check a > > Linux and Android sys/errno.h and they just do #include <errno.h> > > without the #warning. So I just deleted this. (We already #include > > <errno.h> earlier in this file. > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/message_loop/message_loop.cc > > File base/message_loop/message_loop.cc (right): > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/message_loop/message_loop.cc#newcode172 > > base/message_loop/message_loop.cc:172: // Currently NaCl doesn't have a > > UI MessageLoop. > > On 2017/05/17 15:52:53, Nico wrote: > > > Probably want to update this comment. If the expectation is that > > you'll have an > > > UI eventually, also add a TODO > > > > Done. > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/message_loop/message_loop.cc#newcode647 > > base/message_loop/message_loop.cc:647: #endif > > On 2017/05/17 15:52:54, Nico wrote: > > > So this function is just not implemented on fuchsia and will cause a > > linker > > > error if used? (Seems fine.) > > > > Yes, just a link failure right now. I think it'll have to be a Fuchsia > > syscall to do an fd watch, but depends on MessageLoop which I'm not > > clear on the implementation for yet. > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/test/launcher/test_launcher.cc > > File base/test/launcher/test_launcher.cc (right): > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/test/launcher/test_launcher.cc#newcode527 > > base/test/launcher/test_launcher.cc:527: #if defined(OS_POSIX) && > > !defined(OS_FUCHSIA) > > On 2017/05/17 15:52:54, Nico wrote: > > > Do you expect to never need this? If so, why not? (Add comment.) If > > you think we > > > should add it later, add TODO. > > > > Done. > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/trace_event/malloc_dump_provider.cc > > File base/trace_event/malloc_dump_provider.cc (right): > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > base/trace_event/malloc_dump_provider.cc#newcode246 > > base/trace_event/malloc_dump_provider.cc:246: // TODO(scottmg): Port, > > see https://crbug.com/706592. > > On 2017/05/17 15:52:54, Nico wrote: > > > Please use TODO(fuchsia) for all these, for greppability > > > > Done. > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > build/config/compiler/BUILD.gn > > File build/config/compiler/BUILD.gn (right): > > > > https://codereview.chromium.org/2884353004/diff/60001/ > > build/config/compiler/BUILD.gn#newcode1094 > > build/config/compiler/BUILD.gn:1094: if ((is_linux && target_cpu == > > "x86") || is_fuchsia) { > > On 2017/05/17 15:52:54, Nico wrote: > > > 1.) If this happens only in ICU, we should suppress it only there, not > > globally. > > > 2.) Why does this happen for ICU only for fuchsia? > > > > Hmm, #2 is a good question... here's the command lines > > https://gist.github.com/sgraham/c806b636d0d58df4f534d100960df103 > > > > Digging into the flags a bit (%s/ /^M, %!sort, %!uniq, diff) I get this: > > > > [scottmg:~]$ diff linux.txt fuchsia.txt > > 2,3c2,3 > > < --sysroot=../../build/linux/debian_jessie_amd64-sysroot > > < -B../../third_party/binutils/Linux_x64/Release/bin > > --- > > > --sysroot=../../third_party/fuchsia-sdk/sysroot/x86_64-fuchsia > > > --target=x86_64-fuchsia > > 12a13 > > > -DNO_TCMALLOC > > 15a17 > > > -DSYSROOT_VERSION=c831fd41032acfe5ce360cd0459511a0ec3338eb > > 18,24d19 > > < -DUSE_AURA=1 > > < -DUSE_CAIRO=1 > > < -DUSE_GLIB=1 > > < -DUSE_NSS_CERTS=1 > > < -DUSE_PANGO=1 > > < -DUSE_UDEV > > < -DUSE_X11=1 > > 52d46 > > < -Wno-deprecated > > 84d77 > > < -load > > 90,91c83 > > < -pthread > > < -std=gnu++11 > > --- > > > -std=c++11 > > 94d85 > > < > > ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so > > 96d86 > > < check-ipc > > > > I was hoping it was -std=gnu++11 vs c++11 but it's actually that I > > didn't turn on all of the -Wno-deprecated > > https://cs.chromium.org/chromium/src/build/config/compiler/BUILD.gn?rcl= > > 5f7ac3af3be63d35ecba7c6814c842f4d1c12da3&l=1211 > > "for hash_map". > > > > https://codereview.chromium.org/2884353004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I'm not sure which flag it is now. I tried removing -Wno-deprecated and everything seems fine still on Linux , but complains about register on Fuchsia. I tried switching Fuchsia to -std=gnu++11, but no difference. [scottmg:~]$ diff linux.txt fuchsia.txt 2,3c2,3 < --sysroot=../../build/linux/debian_jessie_amd64-sysroot < -B../../third_party/binutils/Linux_x64/Release/bin --- > --sysroot=../../third_party/fuchsia-sdk/sysroot/x86_64-fuchsia > --target=x86_64-fuchsia 12a13 > -DNO_TCMALLOC 15a17 > -DSYSROOT_VERSION=c831fd41032acfe5ce360cd0459511a0ec3338eb 18,24d19 < -DUSE_AURA=1 < -DUSE_CAIRO=1 < -DUSE_GLIB=1 < -DUSE_NSS_CERTS=1 < -DUSE_PANGO=1 < -DUSE_UDEV < -DUSE_X11=1 83d77 < -load 89d82 < -pthread 92,93c85 < ../../third_party/llvm-build/Release+Asserts/bin/clang++ < ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so --- > ./../third_party/llvm-build/Release+Asserts/bin/clang++ 95d86 < check-ipc
On 2017/05/17 20:58:12, scottmg wrote: > I'm not sure which flag it is now. I tried removing -Wno-deprecated and > everything seems fine still on Linux , but complains about register on Fuchsia. > > I tried switching Fuchsia to -std=gnu++11, but no difference. > > [scottmg:~]$ diff linux.txt fuchsia.txt > 2,3c2,3 > < --sysroot=../../build/linux/debian_jessie_amd64-sysroot > < -B../../third_party/binutils/Linux_x64/Release/bin > --- > > --sysroot=../../third_party/fuchsia-sdk/sysroot/x86_64-fuchsia > > --target=x86_64-fuchsia > 12a13 > > -DNO_TCMALLOC > 15a17 > > -DSYSROOT_VERSION=c831fd41032acfe5ce360cd0459511a0ec3338eb > 18,24d19 > < -DUSE_AURA=1 > < -DUSE_CAIRO=1 > < -DUSE_GLIB=1 > < -DUSE_NSS_CERTS=1 > < -DUSE_PANGO=1 > < -DUSE_UDEV > < -DUSE_X11=1 > 83d77 > < -load > 89d82 > < -pthread > 92,93c85 > < ../../third_party/llvm-build/Release+Asserts/bin/clang++ > < ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so > --- > > ./../third_party/llvm-build/Release+Asserts/bin/clang++ > 95d86 > < check-ipc ... ooooohh, oops. The only usage of "register" in ICU file is inside #if !defined(U_WCHAR_IS_UTF16) && !defined(U_WCHAR_IS_UTF32) I'm guessing one of those probably ought to be defined on Fuchsia. I think I might be able to remove the flag all the same https://codereview.chromium.org/2894473002/ though not all builds have come back yet.
On 2017/05/17 20:58:53, scottmg wrote: > On 2017/05/17 20:58:12, scottmg wrote: > > I'm not sure which flag it is now. I tried removing -Wno-deprecated and > > everything seems fine still on Linux , but complains about register on > Fuchsia. > > > > I tried switching Fuchsia to -std=gnu++11, but no difference. > > > > [scottmg:~]$ diff linux.txt fuchsia.txt > > 2,3c2,3 > > < --sysroot=../../build/linux/debian_jessie_amd64-sysroot > > < -B../../third_party/binutils/Linux_x64/Release/bin > > --- > > > --sysroot=../../third_party/fuchsia-sdk/sysroot/x86_64-fuchsia > > > --target=x86_64-fuchsia > > 12a13 > > > -DNO_TCMALLOC > > 15a17 > > > -DSYSROOT_VERSION=c831fd41032acfe5ce360cd0459511a0ec3338eb > > 18,24d19 > > < -DUSE_AURA=1 > > < -DUSE_CAIRO=1 > > < -DUSE_GLIB=1 > > < -DUSE_NSS_CERTS=1 > > < -DUSE_PANGO=1 > > < -DUSE_UDEV > > < -DUSE_X11=1 > > 83d77 > > < -load > > 89d82 > > < -pthread > > 92,93c85 > > < ../../third_party/llvm-build/Release+Asserts/bin/clang++ > > < ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so > > --- > > > ./../third_party/llvm-build/Release+Asserts/bin/clang++ > > 95d86 > > < check-ipc > > ... ooooohh, oops. The only usage of "register" in ICU file is inside > > #if !defined(U_WCHAR_IS_UTF16) && !defined(U_WCHAR_IS_UTF32) > > I'm guessing one of those probably ought to be defined on Fuchsia. I think I > might be able to remove the flag all the same > https://codereview.chromium.org/2894473002/ though not all builds have come back > yet. Wow. Apparently one needs to define https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/pl... a total ordering amongst operating systems https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/pl... https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/pl... I don't even.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
OK, removed -Wno-deprecated entirely, and Fuchsia ICU change is elsewhere. PTanotherL.
LGTM!
The CQ bit was checked by scottmg@chromium.org
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": 140001, "attempt_start_ts": 1495130381875470,
"parent_rev": "c705be48a99fc38fd0144481ad11ca305ba0005a", "commit_rev":
"6ea9ff3e27ed759a26158a6e71fd3d9ea7789aeb"}
Message was sent while issue was closed.
Description was changed from ========== fuchsia: base_unittests (mostly) compiling Things that are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, signal handling in test runner, and backtraces, all of which will need to be implemented in Fuchsia-specific ways in subsequent patches, rather than using the Linux-y/Posix-y ways. Two functions in trace_event are also stubbed for now, not for any good reason, just because I'm not sure how to implement them yet, and this gets things compiling. BUG=706592 ========== to ========== fuchsia: base_unittests (mostly) compiling Things that are compiled out of base at the moment (without replacements implemented in this CL): message loop, process control, signal handling in test runner, and backtraces, all of which will need to be implemented in Fuchsia-specific ways in subsequent patches, rather than using the Linux-y/Posix-y ways. Two functions in trace_event are also stubbed for now, not for any good reason, just because I'm not sure how to implement them yet, and this gets things compiling. BUG=706592 Review-Url: https://codereview.chromium.org/2884353004 Cr-Commit-Position: refs/heads/master@{#472983} Committed: https://chromium.googlesource.com/chromium/src/+/6ea9ff3e27ed759a26158a6e71fd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/chromium/src/+/6ea9ff3e27ed759a26158a6e71fd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
