Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519053003:
Add a GN override to make building fuzzer targets optional. (Closed)
DescriptionAdd a GN override to make building fuzzer targets optional.
Embedders like PDFium have no interest in building them.
Patch Set 1 #
Messages
Total messages: 18 (7 generated)
The CQ bit was checked by thestig@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...
Description was changed from ========== Add a GN override to make building fuzzer targets optional. ========== to ========== Add a GN override to make building fuzzer targets optional. Embedders like PDFium have no interest in building them. ==========
thestig@chromium.org changed reviewers: + kozyatinskiy@chromium.org, machenbach@chromium.org
Now that the existing overrides got cleaned up a bit, I would like to add mine. The Chromium change to add the override variable is here: https://codereview.chromium.org/2521653003 PDFium rolls DEPS for V8 manually, so the V8 DEPS roll and new GN override variable can be added at the same time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
On 2016/11/21 19:50:14, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...) We've also got a pile of code from obj/v8/test/cctest and obj/v8/test/unittests that gets compiled during the pdfium build. V8 folks, can these files be safely made optional and skipped during our build as well?
Ping. This CL is obviously out of date, but I'm wondering if the concept is ok. If yes, I'll update the CL.
machenbach@chromium.org changed reviewers: + jochen@chromium.org
I'm in favor of keeping the simple_fuzzer targets V8-side only. Jochen had concerns in https://codereview.chromium.org/2619743002/ about a more generic approach to this. WDYT jochen, the simple_fuzzer_* targets are only V8-side smoke tests and all have a proper libfuzzer counterpart on the chrome-side anyways. Maybe we can keep one of the simple fuzzer targets as a representative for the others? If you want to work on this change please have a look at https://codereview.chromium.org/2619743002/ first (only the parts concerning fuzzer targets). I think this can be done with a 3-4 line change. Also, in the mean time, the way build overrides work has changed.
Why is PDFium building the fuzzers? Shouldn't it just depend on v8:libv8 or whatever the target is called? I'd like to get to a point where we can do v8 development in a chromium checkout and for that it is required to have all testing targets build
On 2017/03/06 12:47:30, jochen wrote: > Why is PDFium building the fuzzers? Shouldn't it just depend on v8:libv8 or > whatever the target is called? > > I'd like to get to a point where we can do v8 development in a chromium checkout > and for that it is required to have all testing targets build If you build the "all" target or don't specify a target, you always build all targets that exist. Independent on what your project depends on. The (lib)fuzzers have a special status IMO as they are real libfuzzers in Chromium and dummy fuzzers in V8 stand-alone. But currently we have the real libfuzzer and the dummy targets in chromium.
On 2017/03/06 at 14:35:59, machenbach wrote: > On 2017/03/06 12:47:30, jochen wrote: > > Why is PDFium building the fuzzers? Shouldn't it just depend on v8:libv8 or > > whatever the target is called? > > > > I'd like to get to a point where we can do v8 development in a chromium checkout > > and for that it is required to have all testing targets build > > If you build the "all" target or don't specify a target, you always build all targets that exist. Independent on what your project depends on. The (lib)fuzzers have a special status IMO as they are real libfuzzers in Chromium and dummy fuzzers in V8 stand-alone. But currently we have the real libfuzzer and the dummy targets in chromium. they're not dummies, but can be used to run regression tests
On 2017/03/06 14:39:45, jochen wrote: > On 2017/03/06 at 14:35:59, machenbach wrote: > > On 2017/03/06 12:47:30, jochen wrote: > > > Why is PDFium building the fuzzers? Shouldn't it just depend on v8:libv8 or > > > whatever the target is called? > > > > > > I'd like to get to a point where we can do v8 development in a chromium > checkout > > > and for that it is required to have all testing targets build > > > > If you build the "all" target or don't specify a target, you always build all > targets that exist. Independent on what your project depends on. The > (lib)fuzzers have a special status IMO as they are real libfuzzers in Chromium > and dummy fuzzers in V8 stand-alone. But currently we have the real libfuzzer > and the dummy targets in chromium. > > they're not dummies, but can be used to run regression tests Which we already do in V8, right? What benefit do we have to also run those in Chromium? I.e. is it likely that a chromium-side change can break those? In general, I agree that limiting the targets on the V8-side is a low-hanging workaround and not a good solution. The solution here'd be to teach all recipes everywhere and in all downstream projects to not build "all" but only selected targets.
On 2017/03/06 at 14:50:12, machenbach wrote: > On 2017/03/06 14:39:45, jochen wrote: > > On 2017/03/06 at 14:35:59, machenbach wrote: > > > On 2017/03/06 12:47:30, jochen wrote: > > > > Why is PDFium building the fuzzers? Shouldn't it just depend on v8:libv8 or > > > > whatever the target is called? > > > > > > > > I'd like to get to a point where we can do v8 development in a chromium > > checkout > > > > and for that it is required to have all testing targets build > > > > > > If you build the "all" target or don't specify a target, you always build all > > targets that exist. Independent on what your project depends on. The > > (lib)fuzzers have a special status IMO as they are real libfuzzers in Chromium > > and dummy fuzzers in V8 stand-alone. But currently we have the real libfuzzer > > and the dummy targets in chromium. > > > > they're not dummies, but can be used to run regression tests > > Which we already do in V8, right? What benefit do we have to also run those in Chromium? I.e. is it likely that a chromium-side change can break those? I want to get to the point where you can use run-tests.py from a chromium checkout, and having "all" mean different things means that we'll need special cases all over the place (and ppl that would use a chromium checkout for v8 development will miss out running those test) > > In general, I agree that limiting the targets on the V8-side is a low-hanging workaround and not a good solution. The solution here'd be to teach all recipes everywhere and in all downstream projects to not build "all" but only selected targets.
> I want to get to the point where you can use run-tests.py from a chromium > checkout, and having "all" mean different things means that we'll need special > cases all over the place (and ppl that would use a chromium checkout for v8 > development will miss out running those test) > OK, you convinced me for now. Then, I suggest the inlined real solution below. This CL should be abandoned then. > > > > In general, I agree that limiting the targets on the V8-side is a low-hanging > workaround and not a good solution. The solution here'd be to teach all recipes > everywhere and in all downstream projects to not build "all" but only selected > targets.
Message was sent while issue was closed.
On 2017/03/06 15:01:49, Michael Achenbach wrote: > > I want to get to the point where you can use run-tests.py from a chromium > > checkout, and having "all" mean different things means that we'll need special > > cases all over the place (and ppl that would use a chromium checkout for v8 > > development will miss out running those test) > > > > OK, you convinced me for now. Then, I suggest the inlined real solution below. > This CL should be abandoned then. > > > > > > > In general, I agree that limiting the targets on the V8-side is a > low-hanging > > workaround and not a good solution. The solution here'd be to teach all > recipes > > everywhere and in all downstream projects to not build "all" but only selected > > targets. https://pdfium-review.googlesource.com/3562 then? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
