ci: Bootstrap basic Rust CI#13
Conversation
Signed-off-by: tison <wander4096@gmail.com>
| # under the License. | ||
|
|
||
| [default.extend-words] | ||
| "NUMER" = "NUMER" |
There was a problem hiding this comment.
Ignore l1_numer/RESIZE_NUMER and others. Not sure if they are typos or special words.
There was a problem hiding this comment.
"numer" stands for numerator, I've copied the constants from datasketches-cpp (https://github.com/apache/datasketches-cpp/blob/7bb979d3ef8929e235bcd22d67579e1f695f6ecd/hll/include/CubicInterpolation-internal.hpp#L127-L130)
We can opt for "numerator", I think it's better anyway.
There was a problem hiding this comment.
OK. I'd leave the workaround here and let's handle the naming in a follow-up PR if necessary.
There was a problem hiding this comment.
I'd review this config in a follow-up PR.
There was a problem hiding this comment.
We may need INFRA's help to turn on Semantic PR on this repo. But anyway if we agree on using conventional commits, we can leave it here for now.
| cargo run --example hll_usage | ||
|
|
||
| required: | ||
| name: Required |
There was a problem hiding this comment.
We can later add a required status check in .asf.yaml to ensure all PRs can only be merged if CI passed.
| version = "0.1.0" | ||
|
|
||
| edition = "2024" | ||
| rust-version = "1.85.0" |
There was a problem hiding this comment.
edition 2024 requires at least MSRV=1.85.
I personally like moving MSRV eagerly because the old toolchain doesn't get maintained anyway. But other library authors would like to stay as low as possible to keep a wide user adoption range, which, in turn, blocks their dependencies from bumping MSRV.
This is another topic we may discuss later.
| # under the License. | ||
|
|
||
| [default.extend-words] | ||
| "NUMER" = "NUMER" |
There was a problem hiding this comment.
"numer" stands for numerator, I've copied the constants from datasketches-cpp (https://github.com/apache/datasketches-cpp/blob/7bb979d3ef8929e235bcd22d67579e1f695f6ecd/hll/include/CubicInterpolation-internal.hpp#L127-L130)
We can opt for "numerator", I think it's better anyway.
Signed-off-by: tison <wander4096@gmail.com>
| msrv: | ||
| name: Resolve MSRV | ||
| runs-on: ubuntu-24.04 | ||
| outputs: | ||
| rust-versions: ${{ steps.metadata.outputs.rust-versions }} | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - name: Delete rust-toolchain.toml | ||
| run: rm rust-toolchain.toml | ||
| - name: Install toolchain | ||
| uses: dtolnay/rust-toolchain@nightly | ||
| - id: metadata | ||
| run: | | ||
| msrv=$(cargo metadata --format-version 1 --no-deps | jq -r '.packages[] | select(.name == "datasketches") | .rust_version') | ||
| echo "MSRV: $msrv" | ||
| echo "rust-versions=[\"${msrv}\", \"stable\"]" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Ref - #13 (comment)
Let me see if I can get this shell puzzle a bit more understandable.
And the job's & step's naming ...
There was a problem hiding this comment.
Use yq over cargo metadata since cargo metadata can hardly tell what is "our crate". (You must hardcode the crate name as above)
msrv=$(yq '.package.rust-version' Cargo.toml)
Directly read from Cargo.toml. The only ceveat is that if later we move to a workspace setup, it would be msrv=$(yq '.workspace.package.rust-version' Cargo.toml) like what I'm testing on fast/logforth#203.
Signed-off-by: tison <wander4096@gmail.com>
|
LGTM, good for merge ? |
|
Thanks for the MSRV logic. It looks really good! Good to merge on my side! |
|
https://github.com/apache/datasketches-rust/actions/runs/20229178951 Seems like some of the steps are not approved |
Let me do some investigation. It's due to INFRA's actions approval policy and let me see how to work it around. |
4 participants
This closes #12
cc @notfilippo @freakyzoidberg