Treat warnings as errors #379

kt3k · 2018-07-18T01:40:56Z

This resolves #374.

This PR depends on #371. I'm going to rebase it if #371 gets merged.

ry · 2018-07-18T16:59:45Z

build_extra/flatbuffers/BUILD.gn

@@ -13,6 +13,7 @@ config("flatbuffers_config") {
 cflags = [
 "-Wno-exit-time-destructors",
 "-Wno-header-hygiene",
+ "-Wno-return-type", # TODO: rust branch of flatbuffers has this warning. this should be removed when the branch fixed.


Wrap at 80 characters. ./tools/format.py should take care of this.

ry · 2018-07-18T17:00:58Z

build_extra/rust/rust.gni

@@ -40,6 +40,7 @@ template("run_rustc") {

 args = [
 rebase_path(source_root, root_build_dir),
+ "-Dwarnings",


Nice. Can you add this conditionally based on treat_warnings_as_errors ?

ry · 2018-07-18T17:02:12Z

build_extra/rust/BUILD.gn

+ # suppress some warnings
+ "-Aunused-imports",
+ "-Adeprecated",
+ ]


Why does this module have these warnings? Do the authors not care (seems unlikely) or are we compiling it with the wrong cfg? Can you investigate further? If it's their issue, please open an issue with them and link to the issue here.

They seem caused by the line use std::ascii::AsciiExt;. The module seems deprecated at recent version of rust. Their build of master branch also shows the same warnings. So I think it's not a problem of our configuration.

I think this should be fixed in their repository. I filed an issue there. servo/rust-url#455

ry · 2018-07-18T17:03:07Z

build_extra/rust/rust.gni

@@ -102,6 +103,10 @@ template("run_rustc") {
 }
 }

+ if (defined(invoker.args)) {
+ args += invoker.args
+ }


I'd prefer not to add this... but it's fine if you can't fix the warnings in percent_encoding

ry · 2018-07-18T17:03:38Z

Great! Thank you!

refs: denoland#379

refs: denoland#374

refs: denoland#379

ry · 2018-07-19T02:23:29Z

.gn

@@ -25,7 +25,7 @@ default_args = {

 is_component_build = false
 symbol_level = 1
- treat_warnings_as_errors = false
+ treat_warnings_as_errors = true


Remove this line?

ry · 2018-07-19T02:24:22Z

build_extra/rust/BUILD.gn

+ # TODO: Suppress some warnings at this moment
+ # This should be removed when it's fixed in servo/rust-url repository
+ "-Aunused-imports",
+ "-Adeprecated",


Link to the issue you created here

refs: denoland#379

kt3k · 2018-07-19T04:41:55Z

I reverted the following part because it doesn't seem working.

    if (defined(treat_warnings_as_errors) && treat_warnings_as_errors) {
      args += [ "-Dwarnings" ]
    }

(defined(treat_warnings_as_errors) seems evaluated always false). I couldn't find a way to use that build arg in this scope. I left a TODO comment instead.

ry · 2018-07-19T12:58:10Z

LGTM

@piscisaureus please check this works for you.

piscisaureus · 2018-07-20T00:58:40Z

It doesn't work for me. On linux I get a lot of errors like this:

clang++: error: argument unused during compilation: '-isystem ../../buildtools/third_party/libc++abi/trunk/include' [-Werror,-Wunused-command-line-argument]

On windows it breaks here:

C:/programming/depot_tools/win_tools-2_7_6_bin/python/bin/python.exe ../../tools/run_rustc.py ../../third_party/rust_crates/registry/src/github.com-1ecc6299db9ec823/libc-0.2.42/src/lib.rs --crate-name=libc --crate-type=rlib -Dwarnings --emit=link=obj/build_extra/rust/liblibc.rlib --emit=dep-info=obj/build_extra/rust/libc.d --depfile=obj/build_extra/rust/libc.d --output_file=obj/build_extra/rust/liblibc.rlib -g --cfg "feature=\"use_std\""
error: unused macro definition
  --> ../../third_party/rust_crates/registry/src/github.com-1ecc6299db9ec823/libc-0.2.42/src\macros.rs:51:1
   |
51 | / macro_rules! f {
52 | |     ($(pub fn $i:ident($($arg:ident: $argty:ty),*) -> $ret:ty {
53 | |         $($body:stmt);*
54 | |     })*) => ($(
...  |
66 | |     )*)
67 | | }
   | |_^
   |
   = note: `-D unused-macros` implied by `-D warnings`

kt3k · 2018-07-20T01:16:58Z

@piscisaureus
Thanks for checking! I'll investigate them.

piscisaureus · 2018-07-20T01:20:35Z

@kt3k I have to warn you, I do get a lot more warning, especially on linux.
However any warning you can fix is great!

piscisaureus@pumpkin:~/d/deno$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 16.04.3 LTS
Release:        16.04
Codename:       xenial

ry · 2018-07-20T01:32:55Z

@kt3k if it gets difficult to keep up with rebases we can land a partial fix now with these changes but without turning on treat_warnings_as_errors.

kt3k · 2018-07-20T15:03:00Z

The build args in .travis.yml (use_custom_libcxx=false use_sysroot=false) seem suppressing a lot of warnings (such as unused-command-line-argument) on ubuntu, but I'm still seeing many...

I'm going to try some more variations for a while.

kt3k · 2018-07-21T05:46:36Z

@ry @piscisaureus
I reverted the arg treat_warnings_as_errors for now. I'd like to merge at this point.

When I set use_allocator="none" and is_clang=false on ubuntu (16.04) as well as the above args, then it reduces the warnings further but it causes different warnings in deno's source code, such as unused-variable and unused-result. If I try to suppress those, maybe I need to write g++ specific pragmas, but I'm not sure that's the right direction to go.

ry

LGTM

kt3k changed the base branch from code_fetch3 to master July 18, 2018 06:11

kt3k force-pushed the feature/error-on-warnings branch from f608b1f to e2eb270 Compare July 18, 2018 06:15

ry reviewed Jul 18, 2018

View reviewed changes

kt3k added a commit to kt3k/deno that referenced this pull request Jul 19, 2018

chore: update build configuration

68b4a3e

refs: denoland#379

kt3k added 2 commits July 19, 2018 11:22

chore: error on warnings

43be9d5

refs: denoland#374

chore: update build configuration

6321063

refs: denoland#379

kt3k force-pushed the feature/error-on-warnings branch from 68b4a3e to 6321063 Compare July 19, 2018 02:22

ry reviewed Jul 19, 2018

View reviewed changes

chore: update build settings

852fe27

refs: denoland#379

ry requested a review from piscisaureus July 19, 2018 12:58

chore: revert treat_warnings_as_errors to false

12e7c90

ry approved these changes Jul 21, 2018

View reviewed changes

ry merged commit 3563638 into denoland:master Jul 21, 2018

kt3k deleted the feature/error-on-warnings branch July 21, 2018 15:02

kt3k mentioned this pull request Aug 14, 2018

Add rust_treat_warnings_as_errors build arg #517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat warnings as errors #379

Treat warnings as errors #379

kt3k commented Jul 18, 2018 •

edited

Loading

ry Jul 18, 2018

ry Jul 18, 2018

ry Jul 18, 2018

kt3k Jul 19, 2018

ry Jul 18, 2018

ry commented Jul 18, 2018

ry Jul 19, 2018

ry Jul 19, 2018

kt3k commented Jul 19, 2018

ry commented Jul 19, 2018

piscisaureus commented Jul 20, 2018

kt3k commented Jul 20, 2018

piscisaureus commented Jul 20, 2018

ry commented Jul 20, 2018

kt3k commented Jul 20, 2018

kt3k commented Jul 21, 2018

ry left a comment

Treat warnings as errors #379

Treat warnings as errors #379

Conversation

kt3k commented Jul 18, 2018 • edited Loading

ry Jul 18, 2018

Choose a reason for hiding this comment

ry Jul 18, 2018

Choose a reason for hiding this comment

ry Jul 18, 2018

Choose a reason for hiding this comment

kt3k Jul 19, 2018

Choose a reason for hiding this comment

ry Jul 18, 2018

Choose a reason for hiding this comment

ry commented Jul 18, 2018

ry Jul 19, 2018

Choose a reason for hiding this comment

ry Jul 19, 2018

Choose a reason for hiding this comment

kt3k commented Jul 19, 2018

ry commented Jul 19, 2018

piscisaureus commented Jul 20, 2018

kt3k commented Jul 20, 2018

piscisaureus commented Jul 20, 2018

ry commented Jul 20, 2018

kt3k commented Jul 20, 2018

kt3k commented Jul 21, 2018

ry left a comment

Choose a reason for hiding this comment

kt3k commented Jul 18, 2018 •

edited

Loading