alamb commented on code in PR #13876:
URL: https://github.com/apache/datafusion/pull/13876#discussion_r1896174869
##########
.github/actions/setup-rust-runtime/action.yaml:
##########
@@ -34,5 +34,6 @@ runs:
echo "RUSTC_WRAPPER=sccache" >> $GITHUB_ENV
echo "SCCACHE_GHA_ENABLED=true" >> $GITHUB_ENV
echo "RUST_BACKTRACE=1" >> $GITHUB_ENV
+ echo "CARGO_INCREMENTAL=false" >> $GITHUB_ENV
Review Comment:
It seems like this may be redundant with the line below (or if the line
below isn't working. perhaps we could remove it 🤔 )
##########
.github/workflows/rust.yml:
##########
@@ -288,17 +318,20 @@ jobs:
mv *.tbl ../datafusion/sqllogictest/test_files/tpch/data
- name: Verify that benchmark queries return expected results
run: |
+ # increase stack size to fix stack overflow
+ export RUST_MIN_STACK=20971520
Review Comment:
I think we should have fixed this now with the recursive protection feature
-- do we still need to set the min stack size?
##########
.github/workflows/rust.yml:
##########
@@ -347,21 +385,23 @@ jobs:
# cd datafusion-cli
# cargo test --lib --tests --bins --all-features
- macos:
- name: cargo test (macos)
- runs-on: macos-latest
- steps:
- - uses: actions/checkout@v4
- with:
- submodules: true
- - name: Setup Rust toolchain
- uses: ./.github/actions/setup-macos-builder
- - name: Run tests (excluding doctests)
- shell: bash
- run: |
- cargo test --lib --tests --bins --features avro,json,backtrace
- cd datafusion-cli
- cargo test --lib --tests --bins --all-features
+# Commenting out intel mac build as so few users would ever use it
Review Comment:
maybe we can also link to https://github.com/apache/datafusion/issues/13846
##########
Cargo.toml:
##########
@@ -170,6 +170,17 @@ overflow-checks = false
panic = 'unwind'
rpath = false
+[profile.ci]
+inherits = "dev"
+incremental = false
+
+# ci turns off debug info, etc for dependencies to allow for smaller binaries
making caching more effective
+[profile.ci.package."*"]
+debug = false
Review Comment:
We also found this setting particularly effective for speeding up our CI in
influxdb_iox: https://doc.rust-lang.org/cargo/reference/profiles.html#debug
##########
.github/actions/setup-builder/action.yaml:
##########
@@ -42,6 +42,8 @@ runs:
"${RETRY[@]}" rustup component add rustfmt
- name: Configure rust runtime env
uses: ./.github/actions/setup-rust-runtime
+ - name: Setup Rust cache
+ uses: Swatinem/rust-cache@v2
Review Comment:
This is basically my only concern with the PR (using a third-party github
action)
I took a brief look through https://github.com/Swatinem/rust-cache and it
seems like it does non trivial work (there is a lot of code) and it says:
> It is also most effective for repositories with a Cargo.lock file. Library
repositories with only a Cargo.toml file have limited benefits, as cargo will
always use the most up-to-date dependency versions, which may not be cached.
I made an experimental PR to see how much this particular action saves in
terms of time:
- https://github.com/apache/datafusion/pull/13889
If it doesn't save a huge amount of time, I think we should consider not
using it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
