[libfuzzer] Turn warnings from zipfile into errors while archiving seed corpus.

[libfuzzer] Turn warnings from zipfile into errors while archiving seed corpus.

R=aizatsky@chromium.org, inferno@chromium.org, ochang@chromium.org, thakis@chromium.org
BUG=653920
Committed: https://crrev.com/0aa34793db987eec0518583d2a03f72ce9be8f4a
Cr-Commit-Position: refs/heads/master@{#435903}

[inferno, 2016-11-21 19:27:16]

I don't understand why do we have duplicate files in first place ? That seems
wrong and should be fixed, instead of disabling warnings. Warning can show sign
of bugs and other warnings might be more important than this one.

[mmoroz, 2016-11-21 22:52:09]

On 2016/11/21 19:27:16, inferno wrote:
> I don't understand why do we have duplicate files in first place ? That seems
> wrong and should be fixed, instead of disabling warnings. Warning can show
sign
> of bugs and other warnings might be more important than this one.

I wrote a long comment for this but accidentally closed the tab. Summary of my
lost comment (there were some investigations of the issue described):

Abhishek, I would like to agree with you.

I don't understand how to reproduce the issue during a normal workflow. To
reproduce it, I duplicated the following line:
https://cs.chromium.org/chromium/src/testing/libfuzzer/archive_corpus.py?l=35

It caused the warning to be printed for every seed_corpus file. Also it caused
every corpus file to be duplicated in ${fuzzer_name}_seed_corpus.zip archive.
Regardless of the reason why this happens during a normal workflow (at least two
people reported that), we don't want seed corpus files to be duplicated in the
output archive.

Given that, we need to interrupt the build in case of the warning happening and
print some meaningful error. To be honest, I don't know what to print, since I
cannot reproduce it. Should we print "Ensure that you did `gclient sync &&
gclient runhooks`" or anything else?

I think it can be fine to print that warning message and interrupt the build. A
person who sees that can remove the output archive, or do `gn clean`, or do
something else they missed previously. What do you think?

[Nico, 2016-11-21 22:55:47]

On Mon, Nov 21, 2016 at 2:27 PM, <inferno@chromium.org> wrote:

> I don't understand why do we have duplicate files in first place ? That
> seems
> wrong and should be fixed, instead of disabling warnings. Warning can show
> sign
> of bugs and other warnings might be more important than this one.
>

In that case, it should be an error, not a warning.

Warnings don't make the build fail and are usually missed. When they
frequently happen locally, they teach people to ignore warnings.

If something that fails is important, it should be an error. If it isn't
important, it shouldn't be printed. We shouldn't have warnings.


>
> https://codereview.chromium.org/2520983002/
>

-- 
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.

[mmoroz, 2016-11-22 07:53:01]

On 2016/11/21 22:55:47, Nico wrote:
> On Mon, Nov 21, 2016 at 2:27 PM, <mailto:inferno@chromium.org> wrote:
> 
> > I don't understand why do we have duplicate files in first place ? That
> > seems
> > wrong and should be fixed, instead of disabling warnings. Warning can show
> > sign
> > of bugs and other warnings might be more important than this one.
> >
> 
> In that case, it should be an error, not a warning.
> 
> Warnings don't make the build fail and are usually missed. When they
> frequently happen locally, they teach people to ignore warnings.
> 
> If something that fails is important, it should be an error. If it isn't
> important, it shouldn't be printed. We shouldn't have warnings.
> 
> 
> >
> > https://codereview.chromium.org/2520983002/
> >
> 
> -- 
> 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.

+1. FTR, initially I thought that we _overwrite_ an existing file inside of the
archive in case of warning. That didn't seem to be a problem (assuming that file
could get updated, for example), and I uploaded current PatchSet. But since we
_duplicate_ files in the archive, we'll treat that warning as an error. I'll
update the CL and its summary today. Thanks Abhishek and Nico for taking a look.

[mmoroz, 2016-11-22 11:30:00]

Description was changed from

==========
[libfuzzer] Disable warnings from zipfile while archiving seed corpus.

R=aizatsky@chromium.org, inferno@chromium.org, ochang@chromium.org,
thakis@chromium.org
BUG=653920
==========

to

==========
[libfuzzer] Turn warnings from zipfile into errors while archiving seed corpus.

R=aizatsky@chromium.org, inferno@chromium.org, ochang@chromium.org,
thakis@chromium.org
BUG=653920
==========

[mmoroz, 2016-11-22 11:30:59]

On 2016/11/22 07:53:01, mmoroz wrote:
> On 2016/11/21 22:55:47, Nico wrote:
> > On Mon, Nov 21, 2016 at 2:27 PM, <mailto:inferno@chromium.org> wrote:
> > 
> > > I don't understand why do we have duplicate files in first place ? That
> > > seems
> > > wrong and should be fixed, instead of disabling warnings. Warning can show
> > > sign
> > > of bugs and other warnings might be more important than this one.
> > >
> > 
> > In that case, it should be an error, not a warning.
> > 
> > Warnings don't make the build fail and are usually missed. When they
> > frequently happen locally, they teach people to ignore warnings.
> > 
> > If something that fails is important, it should be an error. If it isn't
> > important, it shouldn't be printed. We shouldn't have warnings.
> > 
> > 
> > >
> > > https://codereview.chromium.org/2520983002/
> > >
> > 
> > -- 
> > 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.
> 
> +1. FTR, initially I thought that we _overwrite_ an existing file inside of
the
> archive in case of warning. That didn't seem to be a problem (assuming that
file
> could get updated, for example), and I uploaded current PatchSet. But since we
> _duplicate_ files in the archive, we'll treat that warning as an error. I'll
> update the CL and its summary today. Thanks Abhishek and Nico for taking a
look.

Please take a look. Now the build stops in case of a warning:

$ ninja -C out/Release -j 2048 stylesheet_contents_fuzzer
ninja: Entering directory `out/Release'
[1/1] Regenerating ninja files
[1203/7944] ACTION
//third_party/WebKit/Source/core:style...nts_fuzzer_seed_corpus(//build/toolchain/linux:clang_x64)
FAILED: stylesheet_contents_fuzzer_seed_corpus.zip 
python ../../testing/libfuzzer/archive_corpus.py --corpus
/usr/local/google/home/mmoroz/Projects/new/chromium/src/third_party/WebKit/LayoutTests/fast/css/resources
--output
/usr/local/google/home/mmoroz/Projects/new/chromium/src/out/Release/stylesheet_contents_fuzzer_seed_corpus.zip
--fuzzer
/usr/local/google/home/mmoroz/Projects/new/chromium/src/out/Release/stylesheet_contents_fuzzer_seed_corpus
Traceback (most recent call last):
  File "../../testing/libfuzzer/archive_corpus.py", line 44, in <module>
    main()
  File "../../testing/libfuzzer/archive_corpus.py", line 40, in main
    z.write(corpus_file, os.path.basename(corpus_file))
  File "/usr/lib/python2.7/zipfile.py", line 1141, in write
    self._writecheck(zinfo)
  File "/usr/lib/python2.7/zipfile.py", line 1092, in _writecheck
    warnings.warn('Duplicate name: %r' % zinfo.filename, stacklevel=3)
UserWarning: Duplicate name: 'show-shoes-vs-pie.png'
[1278/7943] CXX
obj/third_party/WebKit/public/new_wrapper_types_mojo_bindings_cpp_sources/websocket.mojom.o
ninja: build stopped: subcommand failed.

[mmoroz, 2016-11-23 11:12:15]

On 2016/11/22 11:30:59, mmoroz wrote:
> On 2016/11/22 07:53:01, mmoroz wrote:
> > On 2016/11/21 22:55:47, Nico wrote:
> > > On Mon, Nov 21, 2016 at 2:27 PM, <mailto:inferno@chromium.org> wrote:
> > > 
> > > > I don't understand why do we have duplicate files in first place ? That
> > > > seems
> > > > wrong and should be fixed, instead of disabling warnings. Warning can
show
> > > > sign
> > > > of bugs and other warnings might be more important than this one.
> > > >
> > > 
> > > In that case, it should be an error, not a warning.
> > > 
> > > Warnings don't make the build fail and are usually missed. When they
> > > frequently happen locally, they teach people to ignore warnings.
> > > 
> > > If something that fails is important, it should be an error. If it isn't
> > > important, it shouldn't be printed. We shouldn't have warnings.
> > > 
> > > 
> > > >
> > > > https://codereview.chromium.org/2520983002/
> > > >
> > > 
> > > -- 
> > > 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.
> > 
> > +1. FTR, initially I thought that we _overwrite_ an existing file inside of
> the
> > archive in case of warning. That didn't seem to be a problem (assuming that
> file
> > could get updated, for example), and I uploaded current PatchSet. But since
we
> > _duplicate_ files in the archive, we'll treat that warning as an error. I'll
> > update the CL and its summary today. Thanks Abhishek and Nico for taking a
> look.
> 
> Please take a look. Now the build stops in case of a warning:
> 
> $ ninja -C out/Release -j 2048 stylesheet_contents_fuzzer
> ninja: Entering directory `out/Release'
> [1/1] Regenerating ninja files
> [1203/7944] ACTION
>
//third_party/WebKit/Source/core:style...nts_fuzzer_seed_corpus(//build/toolchain/linux:clang_x64)
> FAILED: stylesheet_contents_fuzzer_seed_corpus.zip 
> python ../../testing/libfuzzer/archive_corpus.py --corpus
>
/usr/local/google/home/mmoroz/Projects/new/chromium/src/third_party/WebKit/LayoutTests/fast/css/resources
> --output
>
/usr/local/google/home/mmoroz/Projects/new/chromium/src/out/Release/stylesheet_contents_fuzzer_seed_corpus.zip
> --fuzzer
>
/usr/local/google/home/mmoroz/Projects/new/chromium/src/out/Release/stylesheet_contents_fuzzer_seed_corpus
> Traceback (most recent call last):
>   File "../../testing/libfuzzer/archive_corpus.py", line 44, in <module>
>     main()
>   File "../../testing/libfuzzer/archive_corpus.py", line 40, in main
>     z.write(corpus_file, os.path.basename(corpus_file))
>   File "/usr/lib/python2.7/zipfile.py", line 1141, in write
>     self._writecheck(zinfo)
>   File "/usr/lib/python2.7/zipfile.py", line 1092, in _writecheck
>     warnings.warn('Duplicate name: %r' % zinfo.filename, stacklevel=3)
> UserWarning: Duplicate name: 'show-shoes-vs-pie.png'
> [1278/7943] CXX
>
obj/third_party/WebKit/public/new_wrapper_types_mojo_bindings_cpp_sources/websocket.mojom.o
> ninja: build stopped: subcommand failed.

Ping. The change is very tiny :)

[inferno, 2016-11-23 17:19:21]

lgtm

[Nico, 2016-11-23 17:21:00]

lgtm

[mmoroz, 2016-11-23 19:54:01]

The CQ bit was checked by mmoroz@chromium.org

[commit-bot: I haz the power, 2016-11-23 19:54:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 20:28:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 20:28:11]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmoroz, 2016-11-24 10:37:36]

On 2016/11/23 20:28:11, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Why does it happen... Don't buildbots do 'gn clean' before building?

FAILED: html_preload_scanner_fuzzer_seed_corpus.zip 
python ../../testing/libfuzzer/archive_corpus.py --corpus
/b/c/b/linux/src/third_party/WebKit/LayoutTests/fast/parser --output
/b/c/b/linux/src/out/Debug/html_preload_scanner_fuzzer_seed_corpus.zip --fuzzer
/b/c/b/linux/src/out/Debug/html_preload_scanner_fuzzer_seed_corpus
Traceback (most recent call last):
  File "../../testing/libfuzzer/archive_corpus.py", line 43, in <module>
    main()
  File "../../testing/libfuzzer/archive_corpus.py", line 39, in main
    z.write(corpus_file, os.path.basename(corpus_file))
  File "/usr/lib/python2.7/zipfile.py", line 1141, in write
    self._writecheck(zinfo)
  File "/usr/lib/python2.7/zipfile.py", line 1092, in _writecheck
    warnings.warn('Duplicate name: %r' % zinfo.filename, stacklevel=3)

[mmoroz, 2016-11-24 10:44:41]

The CQ bit was checked by mmoroz@chromium.org

[mmoroz, 2016-11-24 10:44:41]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
inferno@chromium.org
Link to the patchset: https://codereview.chromium.org/2520983002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-11-24 10:45:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 15:08:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 15:08:42]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nico, 2016-11-24 19:36:24]

No, most bots don't do clobber builds.

On Nov 24, 2016 10:08 AM, "commit-bot@chromium.org via
codereview.chromium.org" <reply@chromiumcodereview-hr.appspotmail.com>
wrote:

Try jobs failed on following builders:
linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/
builders/linux_chromium_compile_dbg_ng/builds/198196)

https://codereview.chromium.org/2520983002/

-- 
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.

[mmoroz, 2016-11-29 13:08:07]

On 2016/11/24 19:36:24, Nico wrote:
> No, most bots don't do clobber builds.
> 
> On Nov 24, 2016 10:08 AM, mailto:"commit-bot@chromium.org via
> codereview.chromium.org" <mailto:reply@chromiumcodereview-hr.appspotmail.com>
> wrote:
> 
> Try jobs failed on following builders:
> linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
> http://build.chromium.org/p/tryserver.chromium.linux/
> builders/linux_chromium_compile_dbg_ng/builds/198196)
> 
> https://codereview.chromium.org/2520983002/
> 
> -- 
> 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.

Please take a look once again. More context:
https://bugs.chromium.org/p/chromium/issues/detail?id=653920#c8

[mmoroz, 2016-11-29 13:53:41]

https://codereview.chromium.org/2520983002/diff/100001/testing/libfuzzer/arch...
File testing/libfuzzer/archive_corpus.py (right):

https://codereview.chromium.org/2520983002/diff/100001/testing/libfuzzer/arch...
testing/libfuzzer/archive_corpus.py:40: arcname = '%016d' % i
I thought to use subdirectory_name + '_' + os.path.basename(corpus_file), but
names of corpus files are not important, so that complication wouldn't make
sense.

[aizatsky, 2016-11-29 19:59:53]

Max, libfuzzer now reads the corpus recursively. I believe we can simply zip
everything with directory names.

[mmoroz, 2016-11-29 20:41:04]

On 2016/11/29 19:59:53, aizatsky wrote:
> Max, libfuzzer now reads the corpus recursively. I believe we can simply zip
> everything with directory names.

I thought about it, but not sure how do we manage and how we will manage (when
something changes) corpus directories in CF scripts (we can merge them, re-name,
copy, chose N random testcases, etc). Other complications may happen because of
this.

One more "complication": we already have long paths issues on windows (not for
libFuzzer, but anyway). Subdirectories will make paths longer.

Current solution feels the dumbest and the safest to me, until we don't care
about names of files/directories in seed_corpus :)

[mmoroz, 2016-12-02 09:44:32]

The CQ bit was checked by mmoroz@chromium.org

[mmoroz, 2016-12-02 09:44:32]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
inferno@chromium.org
Link to the patchset: https://codereview.chromium.org/2520983002/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-12-02 09:44:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 10:08:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480671872424170,
"parent_rev": "410d5e05c376923e25420228f55fd0f494e0ca83", "commit_rev":
"5d6bf7aa6074bcdcf6e43100c490ae32d0a2638f"}

[commit-bot: I haz the power, 2016-12-02 10:08:25]

Description was changed from

==========
[libfuzzer] Turn warnings from zipfile into errors while archiving seed corpus.

R=aizatsky@chromium.org, inferno@chromium.org, ochang@chromium.org,
thakis@chromium.org
BUG=653920
==========

to

==========
[libfuzzer] Turn warnings from zipfile into errors while archiving seed corpus.

R=aizatsky@chromium.org, inferno@chromium.org, ochang@chromium.org,
thakis@chromium.org
BUG=653920
==========

[commit-bot: I haz the power, 2016-12-02 10:08:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-02 10:11:17]

Description was changed from

==========
[libfuzzer] Turn warnings from zipfile into errors while archiving seed corpus.

R=aizatsky@chromium.org, inferno@chromium.org, ochang@chromium.org,
thakis@chromium.org
BUG=653920
==========

to

==========
[libfuzzer] Turn warnings from zipfile into errors while archiving seed corpus.

R=aizatsky@chromium.org, inferno@chromium.org, ochang@chromium.org,
thakis@chromium.org
BUG=653920

Committed: https://crrev.com/0aa34793db987eec0518583d2a03f72ce9be8f4a
Cr-Commit-Position: refs/heads/master@{#435903}
==========

[commit-bot: I haz the power, 2016-12-02 10:11:18]

Patchset 6 (id:??) landed as
https://crrev.com/0aa34793db987eec0518583d2a03f72ce9be8f4a
Cr-Commit-Position: refs/heads/master@{#435903}

[inferno, 2016-12-02 14:39:04]

lgtm, since these are corpus files, just numbered names are ok.