Skip to content

ARROW-12224: [Rust] Use stable rust for no default test, clean up CI tests - #9904

Closed
alamb wants to merge 3 commits into
apache:masterfrom
alamb:alamb/cleanup_rust_ci
Closed

ARROW-12224: [Rust] Use stable rust for no default test, clean up CI tests#9904
alamb wants to merge 3 commits into
apache:masterfrom
alamb:alamb/cleanup_rust_ci

Conversation

@alamb

@alambalamb commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

Rationale

  1. As @jorgecarleitao noted on ARROW-12204: [Rust][CI] Reduce size of Rust build artifacts in integration test #9889 (comment), we should be running the check if arrow compiles with stable rust as that is what we target for the arrow crate
  2. I noticed that there were several redundant (and inconsistent) settings of RUSTFLAGS
  3. The titles of many of the tests are confusing (to me) as they have a lot of detailed architecture / rust version information before the description of what they are testing

Screen Shot 2021-04-06 at 6 39 19 AM

Changes

  1. Use rust stable for the check that ensures the crate builds without default features
  2. Remove redundant RUSTFLAGS
  3. Change titles of the jobs to consistently start with a description of what they do

Note

This could be three individual PRs, but I wanted to avoid the overhead of three separate JIRA tickets and juggling several concurrent potentially conflicting PRs. I will break it into three individual ones however if reviewers want.

@alamb
alambforce-pushed the alamb/cleanup_rust_ci branch from aa5bd35 to dad8209CompareApril 6, 2021 10:55
@github-actions

Copy link
Copy Markdown

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #9904 (dad8209) into master (dd8cd10) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #9904 +/- ##
=======================================
Coverage 82.71% 82.71% =======================================
Files 256 256 Lines 60186 60186 =======================================
Hits 49780 49780 Misses 10406 10406 

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd8cd10...dad8209. Read the comment docs.

@jorgecarleitaojorgecarleitao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM Thanks a lot!


# build the library, a compilation step used by multiple steps below
linux-build-lib:
name: AMD64 Debian 10 Rust ${{ matrix.rust }} build libraries

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@kszucs has raised in the past the he would prefer to have the same names as other implementations.

pinging him here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the hint @jorgecarleitao -- will wait for @kszucs to weigh in

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer this convention to remain consistent with the rest of the workflows (C++, Python, Ruby, etc.).
I'm fine with using a different naming convention too if you think that helps distinguishing the builds.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think it will help. Let's try this way and see how we like it. Thanks @kszucs 👍

Sign up for freeto 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.

5 participants

@alamb@codecov-io@andygrove@kszucs@jorgecarleitao