Skip to content

chore(build): unify rustflags for all targets#6113

Merged
mergify[bot] merged 8 commits intomainfrom
bz/unify-rustflags-2
Oct 31, 2022
Merged

chore(build): unify rustflags for all targets#6113
mergify[bot] merged 8 commits intomainfrom
bz/unify-rustflags-2

Conversation

@BugenZhao
Copy link
Contributor

@BugenZhao BugenZhao commented Oct 30, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

After rust-lang/cargo#11114, we're able to unify rustflags for all targets.

There are four mutually exclusive sources of extra flags. They are checked in order, with the first one being used:

  1. CARGO_ENCODED_RUSTFLAGS environment variable.
  2. RUSTFLAGS environment variable.
  3. All matching target.<triple>.rustflags and target.<cfg>.rustflags config entries joined together.
  4. build.rustflags config value.

This PR also adds the step of clippy check without test targets in CI.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@codecov
Copy link

codecov bot commented Oct 30, 2022

Codecov Report

Merging #6113 (4935116) into main (49cc50d) will increase coverage by 0.11%.
The diff coverage is 59.18%.

@@            Coverage Diff             @@
##             main    #6113      +/-   ##
==========================================
+ Coverage   74.70%   74.82%   +0.11%     
==========================================
  Files         933      931       -2     
  Lines      149025   148964      -61     
==========================================
+ Hits       111330   111458     +128     
+ Misses      37695    37506     -189     
Flag Coverage Δ
rust 74.82% <59.18%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rc/ctl/src/cmd_impl/hummock/list_version_deltas.rs 0.00% <0.00%> (ø)
src/frontend/src/handler/query.rs 18.75% <0.00%> (-0.78%) ⬇️
src/frontend/src/optimizer/plan_node/stream.rs 54.71% <0.00%> (-1.60%) ⬇️
src/meta/src/hummock/compaction_group/manager.rs 66.55% <0.00%> (-11.01%) ⬇️
src/meta/src/hummock/manager/mod.rs 73.27% <0.00%> (-1.49%) ⬇️
src/meta/src/lib.rs 0.92% <0.00%> (-0.04%) ⬇️
src/meta/src/rpc/service/hummock_service.rs 3.64% <0.00%> (-0.12%) ⬇️
src/rpc_client/src/meta_client.rs 0.00% <0.00%> (ø)
src/utils/pgwire/src/error.rs 0.00% <ø> (ø)
src/utils/pgwire/src/pg_protocol.rs 57.39% <0.00%> (-0.26%) ⬇️
... and 27 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@BugenZhao BugenZhao marked this pull request as ready for review October 30, 2022 14:34
@BugenZhao BugenZhao requested a review from xxchan October 30, 2022 14:46
source ci/scripts/common.env.sh

echo "--- Run clippy check"
cargo clippy --locked -- -D warnings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add it to risedev? Otherwise it might be confusing if risedev c passed but ci failed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could I ask what is the purpose of adding this? Is it to test without sync_point? Or make the check run faster?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add it to risedev?

This will make the checking slower and I'm not sure whether it is worth it. 🥵

Could I ask what is the purpose of adding this?

For example, there're two unused imports in hummock/mod.rs shown as warnings when we build with risedev d, but cannot be checked with --all-targets --all-features.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make the checking slower and I'm not sure whether it is worth it. 🥵

Then I prefer not include it even in CI. Just manually fix it from time to time, or simply don't fix it 🥵

[profile.release]
debug = true
rustflags = ["-Ctarget-cpu=native"] #5367

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, according to the priority list in the PR description, am I right to say that this is considered build.rustflags and hence has lower priority than target.rustflags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new feature named "profile rustflags" which is not mentioned in the doc. It's joined with 3).

     Running `rustc --crate-name regex_syntax --edition=2018 /home/bugenzhao/.cargo/registry/src/github.com-1ecc6299db9ec823/regex-syntax-0.6.27/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -Ctarget-cpu=native -C embed-bitcode=no -C debuginfo=2 -C debug-assertions=off --cfg 'feature="default"' --cfg 'feature="unicode"' --cfg 'feature="unicode-age"' --cfg 'feature="unicode-bool"' --cfg 'feature="unicode-case"' --cfg 'feature="unicode-gencat"' --cfg 'feature="unicode-perl"' --cfg 'feature="unicode-script"' --cfg 'feature="unicode-segment"' -C metadata=9940732c2dd3c8e5 -C extra-filename=-9940732c2dd3c8e5 --out-dir /home/bugenzhao/work/risingwave/target/release/deps -L dependency=/home/bugenzhao/work/risingwave/target/release/deps --cap-lints allow -Ctarget-feature=+avx2 --cfg tokio_unstable -Funused_must_use '-Wclippy::dbg_macro' '-Wclippy::disallowed_methods' '-Wclippy::disallowed_types' '-Wclippy::doc_markdown' '-Wclippy::explicit_into_iter_loop' '-Wclippy::explicit_iter_loop' '-Wclippy::inconsistent_struct_constructor' '-Wclippy::unused_async' '-Wclippy::map_flatten' '-Wclippy::no_effect_underscore_binding' '-Wclippy::await_holding_lock' '-Wrustdoc::broken_intra_doc_links' -Wfuture_incompatible -Wnonstandard_style -Wrust_2018_idioms -Clink-arg=-fuse-ld=lld -Clink-arg=-Wl,--no-rosegment`

Copy link
Contributor

@jon-chuang jon-chuang Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. In this case target-cpu native may not be desirable. As it would mean we may not have cross-target compatible binary, which we would want in a release binary.

We probably only want this flag if the target does not correspond to aarch or x86. We may want to ignore these corner cases (powerpc??).

So maybe we do not need this flag... although I'm not sure if target triple target.x86_64-unknown-linux-gnu also includes x86 on mac and windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Let's remove this flag. I believe we only care about the performance under Linux with x86-64 and aarch64 for now. Let's tweak it in the future, if necessary.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@mergify mergify bot merged commit 566fb9b into main Oct 31, 2022
@mergify mergify bot deleted the bz/unify-rustflags-2 branch October 31, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants