1516829 - Warnings under servo/ break build with Rust Nightly and --enable-warnings-as-errors

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Jan Beich]

Stylo builds a lot of vendored code under servo/ directory. Due to high churn rate there it'd probably be better to disable warnings like under third_party/rust/.

$ rustc -vV
rustc 1.33.0-nightly
binary: rustc
commit-hash: 60e825389db3
commit-date: 2018-12-28
host: x86_64-unknown-freebsd
release: 1.33.0-nightly
LLVM version: 8.0

$ echo "ac_add_options --enable-warnings-as-errors # mimic MOZ_AUTOMATION" >>.mozconfig
$ ./mach build
[...]
error: use of deprecated item 'bitflags::core::str::<impl str>::trim_right': superseded by `trim_end`
   --> servo/components/style/values/computed/font.rs:781:15
    |
781 |         slice.trim_right().to_css(dest)
    |               ^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`

error: use of deprecated item 'bitflags::core::str::<impl str>::trim_left_matches': superseded by `trim_start_matches`
   --> servo/components/style/values/specified/position.rs:695:27
    |
695 |         let rest = self.0.trim_left_matches(HTML_SPACE_CHARACTERS);
    |                           ^^^^^^^^^^^^^^^^^
error: aborting due to 2 previous errors                                                                                                                                                                                       error: Could not compile `style`.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1516829 - Replace trim_{left,right}* with trim_{start,end}*.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jan Beich from comment #0)
> Stylo builds a lot of vendored code under servo/ directory. Due to high
> churn rate there it'd probably be better to disable warnings like under
> third_party/rust/.

The code there is not vendored, fwiw.

> $ echo "ac_add_options --enable-warnings-as-errors # mimic MOZ_AUTOMATION"
> >>.mozconfig

Well, we build with --enable-warnings-as-errors, but not with rust nightly. The same happens when clang introduces a warning if you use a clang version newer than what we use on automation, right?

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab4ecfb8cc17c17b088d0a7508f1b0a00fa8553

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a22cf2ec4f2d
Replace trim_{left,right}* with trim_{start,end}*. r=emilio

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Maybe we can setup a Tier-3 task to build and test with nightly and/or beta, so that we can find this kind of issues quickly.

This would help us tracking upstream changes closely, and help contributors who may use newer versions to be able to build most of the time. Gecko's Rust codebase would also be a good real world testcase for Rust, which can help Rust team as well.

I recall that Travis CI's Rust page mentions[1]:
> The Rust team appreciates testing against the beta and nightly channels, even if you are only targeting stable.

Although most of the time our experience with Rust upgrade was good, I still recall there was once that we faced several regressions which caused us not able to bump the Rust version for several (Rust) releases. Testing latest toolchain should help with that.

This doesn't need to run very frequently, so a T3 task on m-c should be enough. WDYT?


[1] https://docs.travis-ci.com/user/languages/rust/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+11 from comment #5)
> This would help us tracking upstream changes closely, and help contributors
> who may use newer versions to be able to build most of the time.

Well sure, though that only helps people that build with --enable-warnings-as-errors, which I think it's a known may-break flag.


> Although most of the time our experience with Rust upgrade was good, I still
> recall there was once that we faced several regressions which caused us not
> able to bump the Rust version for several (Rust) releases. Testing latest
> toolchain should help with that.

Those were mostly perf regressions right? So they wouldn't be caught by this.

> This doesn't need to run very frequently, so a T3 task on m-c should be
> enough. WDYT?

Sounds fine in any case :)

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)
> (In reply to Xidorn Quan [:xidorn] UTC+11 from comment #5)
> > This would help us tracking upstream changes closely, and help contributors
> > who may use newer versions to be able to build most of the time.
> 
> Well sure, though that only helps people that build with
> --enable-warnings-as-errors, which I think it's a known may-break flag.

Yeah, but I believe people are, or should be, encouraged to use that since that's what we do on CI, otherwise they may need more rounds to get code in since there would be a larger chance to see failures not present locally.

> > Although most of the time our experience with Rust upgrade was good, I still
> > recall there was once that we faced several regressions which caused us not
> > able to bump the Rust version for several (Rust) releases. Testing latest
> > toolchain should help with that.
> 
> Those were mostly perf regressions right? So they wouldn't be caught by this.

Perf regressions can still be caught with talos if we run that :)

I think there were something related to backtrace or symbol stuff... I forgot exactly what was that, though.

> > This doesn't need to run very frequently, so a T3 task on m-c should be
> > enough. WDYT?
> 
> Sounds fine in any case :)

I'll file a bug :)

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a22cf2ec4f2d