Rename and invert skip_unit_tests to build_native_rust_unit_tests [chromium/src : main]
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
To view, visit change 3513974. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
danakj uploaded patch set #2 to this change.
Rename and invert skip_unit_tests to build_native_rust_unit_tests
Since skip_unit_tests was defaulting to true now, instead we rename it
to build_native_rust_unit_tests which defaults to false.
R=luk...@chromium.org
Bug: 1296156
Change-Id: I299e53185e2733d44fdd720d720e7b6840bc596f
Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-rel,android-rust-arm-rel
---
M build/config/rust.gni
M build/rust/cargo_crate.gni
M build/rust/mixed_target.gni
M build/rust/rust_executable.gni
M build/rust/rust_static_library.gni
M build/rust/rust_target.gni
M build/rust/rust_unit_test.gni
M build/rust/tests/BUILD.gn
M build/rust/tests/test_mixed_component/BUILD.gn
M build/rust/tests/test_mixed_executable/BUILD.gn
M build/rust/tests/test_mixed_shared_library/BUILD.gn
M build/rust/tests/test_mixed_static_library/BUILD.gn
M build/rust/tests/test_mixed_testonly_executable/BUILD.gn
M build/rust/tests/test_rust_multiple_dep_versions_exe/v1/BUILD.gn
M build/rust/tests/test_rust_multiple_dep_versions_exe/v2/BUILD.gn
M build/rust/tests/test_variable_static_library/BUILD.gn
M mojo/public/rust/BUILD.gn
M testing/buildbot/waterfalls.pyl
M third_party/rust/aho_corasick/v0_7/BUILD.gn
M third_party/rust/ansi_term/v0_11/BUILD.gn
M third_party/rust/aquamarine/v0_1/BUILD.gn
M third_party/rust/atty/v0_2/BUILD.gn
M third_party/rust/autocxx/v0_16/BUILD.gn
M third_party/rust/autocxx_bindgen/v0_59/BUILD.gn
M third_party/rust/autocxx_engine/v0_16/BUILD.gn
M third_party/rust/autocxx_macro/v0_16/BUILD.gn
M third_party/rust/autocxx_parser/v0_16/BUILD.gn
M third_party/rust/bindgen/v0_59/BUILD.gn
M third_party/rust/bitflags/v1/BUILD.gn
M third_party/rust/cexpr/v0_6/BUILD.gn
M third_party/rust/cfg_if/v1/BUILD.gn
M third_party/rust/clang_sys/v1/BUILD.gn
M third_party/rust/clap/v2/BUILD.gn
M third_party/rust/codespan_reporting/v0_11/BUILD.gn
M third_party/rust/cxx/v1/BUILD.gn
M third_party/rust/cxx_gen/v0_7/BUILD.gn
M third_party/rust/cxxbridge_cmd/v1/BUILD.gn
M third_party/rust/cxxbridge_macro/v1/BUILD.gn
M third_party/rust/either/v1/BUILD.gn
M third_party/rust/env_logger/v0_9/BUILD.gn
M third_party/rust/fastrand/v1/BUILD.gn
M third_party/rust/glob/v0_3/BUILD.gn
M third_party/rust/heck/v0_3/BUILD.gn
M third_party/rust/humantime/v2/BUILD.gn
M third_party/rust/indoc/v1/BUILD.gn
M third_party/rust/itertools/v0_10/BUILD.gn
M third_party/rust/itertools/v0_9/BUILD.gn
M third_party/rust/itoa/v0_4/BUILD.gn
M third_party/rust/itoa/v1/BUILD.gn
M third_party/rust/lazy_static/v1/BUILD.gn
M third_party/rust/lazycell/v1/BUILD.gn
M third_party/rust/libc/v0_2/BUILD.gn
M third_party/rust/libloading/v0_7/BUILD.gn
M third_party/rust/link_cplusplus/v1/BUILD.gn
M third_party/rust/log/v0_4/BUILD.gn
M third_party/rust/memchr/v2/BUILD.gn
M third_party/rust/minimal_lexical/v0_2/BUILD.gn
M third_party/rust/moveit/v0_4/BUILD.gn
M third_party/rust/nom/v7/BUILD.gn
M third_party/rust/once_cell/v1/BUILD.gn
M third_party/rust/peeking_take_while/v0_1/BUILD.gn
M third_party/rust/proc_macro2/v1/BUILD.gn
M third_party/rust/proc_macro_error/v1/BUILD.gn
M third_party/rust/proc_macro_error_attr/v1/BUILD.gn
M third_party/rust/quote/v1/BUILD.gn
M third_party/rust/regex/v1/BUILD.gn
M third_party/rust/regex_syntax/v0_6/BUILD.gn
M third_party/rust/remove_dir_all/v0_5/BUILD.gn
M third_party/rust/rstest/v0_12/BUILD.gn
M third_party/rust/rustc_hash/v1/BUILD.gn
M third_party/rust/rustc_version/v0_4/BUILD.gn
M third_party/rust/rustversion/v1/BUILD.gn
M third_party/rust/ryu/v1/BUILD.gn
M third_party/rust/semver/v1/BUILD.gn
M third_party/rust/serde/v1/BUILD.gn
M third_party/rust/serde_derive/v1/BUILD.gn
M third_party/rust/serde_json/v1/BUILD.gn
M third_party/rust/serde_jsonrc/v0_1/BUILD.gn
M third_party/rust/shlex/v1/BUILD.gn
M third_party/rust/small_ctor/v0_1/BUILD.gn
M third_party/rust/strsim/v0_8/BUILD.gn
M third_party/rust/strum_macros/v0_23/BUILD.gn
M third_party/rust/syn/v1/BUILD.gn
M third_party/rust/tempfile/v3/BUILD.gn
M third_party/rust/termcolor/v1/BUILD.gn
M third_party/rust/textwrap/v0_11/BUILD.gn
M third_party/rust/unicode_segmentation/v1/BUILD.gn
M third_party/rust/unicode_width/v0_1/BUILD.gn
M third_party/rust/unicode_xid/v0_2/BUILD.gn
M third_party/rust/unindent/v0_1/BUILD.gn
M third_party/rust/unzip_n/v0_1/BUILD.gn
M third_party/rust/vec_map/v0_8/BUILD.gn
M third_party/rust/version_check/v0_9/BUILD.gn
M third_party/rust/which/v4/BUILD.gn
M third_party/rust/winapi/v0_3/BUILD.gn
M third_party/rust/winapi_util/v0_1/BUILD.gn
M tools/crates/lib/build_rule.py
M tools/crates/lib/consts.py
98 files changed, 146 insertions(+), 124 deletions(-)
To view, visit change 3513974. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
Patch set 3:Code-Review +1
5 comments:
Patchset:
Thanks! LGTM - PTAL at my comments below but none of them are blocking.
File build/rust/rust_executable.gni:
nit: Is this a good place to mention: 1) first-party Chromium code should typically avoid setting this GN variable, because it's tests should use #[gtest(...)] attribute instead and/or 2) for discussion on when to use #[test] vs #[gtest(...)] please see some-link-to-md-docs-here? WDYT?
File build/rust/tests/test_mixed_component/BUILD.gn:
Patch Set #3, Line 16:
rs_build_native_rust_unit_tests = trueOptional / just an idea: Do we want to prevent copy&paste (i.e. prevent others from assuming that `#[test]` should be used instead of `#[gtest(...)]`) by adding a comment here (and in other similar BUILD.gn) files saying:
# Tests under //build/rust/tests are too low-level to depend on
# `#[gtest(...)]` attribute that should be used for most of the
# other first-party tests in Chromium. To build tests using
# `#[test]` attribute we need to explicitly opt-in by setting the
# `rs_build_native_rust_unit_tests` here.
Too long? Feel free to word-smith (or abandon altogether).
File build/rust/tests/test_rust_multiple_dep_versions_exe/v2/BUILD.gn:
Patch Set #3, Line 14:
falseFascinating. Thanks for catching this and filing a bug.
I guess `rust_unit_tests_group` [1] will be useful for providing a more human-friendly name for the tests (without auto-generated version parts, etc. - e.g. we'll probably end up defining "third_party_crate_tests" test group, right?)
File third_party/rust/aho_corasick/v0_7/BUILD.gn:
Patch Set #3, Line 18:
build_native_rust_unit_tests = falseIn the long-term we plan to run third-party crate tests on bots, right? (e.g. to ensure test coverage on all Chromium-supported platforms) QUESTION: Is there a bug to track this work?
Also - I wonder if this particular bit (`build_native_rust_unit_tests`) is per-crate VS global. It seems like a global knob to me (?). Maybe with a way to override in rare circumstances for individual crates if their tests don't work in Chromium for some reason. QUESTION: If this is global, then do we want to deduplicate this somehow? QUESTION2: How do we configure `rustc` warnings/errors policy (and do we want a unified policy for all 3rd party crates)?
To view, visit change 3513974. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
4 comments:
File build/rust/rust_executable.gni:
nit: Is this a good place to mention: 1) first-party Chromium code should typically avoid setting th […]
Done
File build/rust/tests/test_mixed_component/BUILD.gn:
Patch Set #3, Line 16:
rs_build_native_rust_unit_tests = true
Optional / just an idea: Do we want to prevent copy&paste (i.e. […]
Done, in mixed_target.gni. Also a TODO here.
File build/rust/tests/test_rust_multiple_dep_versions_exe/v2/BUILD.gn:
Patch Set #3, Line 14:
false
Fascinating. Thanks for catching this and filing a bug. […]
Yeah, we should do that I think.
File third_party/rust/aho_corasick/v0_7/BUILD.gn:
Patch Set #3, Line 18:
build_native_rust_unit_tests = false
In the long-term we plan to run third-party crate tests on bots, right? (e.g. […]
I thought there was a bug but couldn't find it, so: https://bugs.chromium.org/p/chromium/issues/detail?id=1304772
Making it global is an interesting thought, but I think it's better to specify it in each crate. That way as you say we can turn on/off crates if they are broken. Also in general globals are a bit weird in GN, and there's low cost here since it's a generated BUILD file, and higher clarity.
3rd party crate warnings are currently suppressed by https://chromium-review.googlesource.com/c/chromium/src/+/3259592 at https://source.chromium.org/chromium/chromium/src/+/main:build/config/compiler/BUILD.gn;drc=d48fd383bdcd9952c690f9b729c38313895d19b0;l=1752.
To view, visit change 3513974. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Patch set 5:Commit-Queue +2
danakj (Gerrit)
Patch set 5:Owners-Override +1Commit-Queue +2
1 comment:
Patchset:
OO for the buildbot, changing a comment
To view, visit change 3513974. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: build/rust/rust_executable.gni
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: build/rust/tests/test_mixed_component/BUILD.gn
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: build/rust/tests/test_rust_static_library_non_standard_arrangement/BUILD.gn
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: build/rust/tests/test_mixed_executable/BUILD.gn
Insertions: 0, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: build/rust/mixed_target.gni
Insertions: 5, Deletions: 11.
The diff is too large to show. Please review the diff.
```
```
The name of the file: build/rust/tests/test_mixed_testonly_executable/BUILD.gn
Insertions: 0, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: build/rust/tests/test_mixed_static_library/BUILD.gn
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: build/rust/tests/test_mixed_shared_library/BUILD.gn
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Rename and invert skip_unit_tests to build_native_rust_unit_tests
Since skip_unit_tests was defaulting to true now, instead we rename it
to build_native_rust_unit_tests which defaults to false.
R=luk...@chromium.org
Bug: 1296156
Change-Id: I299e53185e2733d44fdd720d720e7b6840bc596f
Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-rel,android-rust-arm-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3513974
Reviewed-by: Łukasz Anforowicz <luk...@chromium.org>
Commit-Queue: danakj <dan...@chromium.org>
Owners-Override: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#979351}
---
M build/config/rust.gni
M build/rust/cargo_crate.gni
M build/rust/mixed_target.gni
M build/rust/rust_executable.gni
M build/rust/rust_static_library.gni
M build/rust/rust_target.gni
M build/rust/rust_unit_test.gni
M build/rust/tests/BUILD.gn
M build/rust/tests/test_mixed_component/BUILD.gn
M build/rust/tests/test_mixed_executable/BUILD.gn
M build/rust/tests/test_mixed_shared_library/BUILD.gn
M build/rust/tests/test_mixed_static_library/BUILD.gn
M build/rust/tests/test_mixed_testonly_executable/BUILD.gn
M build/rust/tests/test_rust_exe/BUILD.gn
M build/rust/tests/test_rust_multiple_dep_versions_exe/v1/BUILD.gn
M build/rust/tests/test_rust_multiple_dep_versions_exe/v2/BUILD.gn
M build/rust/tests/test_rust_shared_library/BUILD.gn
M build/rust/tests/test_rust_static_library/BUILD.gn
M build/rust/tests/test_rust_static_library_non_standard_arrangement/BUILD.gn
102 files changed, 181 insertions(+), 134 deletions(-)