Skip to content

ARROW-12444: [Rust] Remove rust - #10096

Closed
jorgecarleitao wants to merge 13 commits into
apache:masterfrom
jorgecarleitao:rm-rust
Closed

ARROW-12444: [Rust] Remove rust#10096
jorgecarleitao wants to merge 13 commits into
apache:masterfrom
jorgecarleitao:rm-rust

Conversation

@jorgecarleitao

@jorgecarleitaojorgecarleitao commented Apr 19, 2021

Copy link
Copy Markdown
Member

Removes rust from this repository and uses git clone to fetch the arrow-rs repo for integration tests.

Each commit is an independent change.

Note: Rust implementations have moved to:

@github-actions

Copy link
Copy Markdown

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The basic idea looks good to me @jorgecarleitao. Thank you.

The only thing I think may be worth a second set of eyes is the archery integration stuff (but perhaps that is because I don't have a great handle on the integration test plan)

Comment threaddev/archery/archery/lang/rust.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure this one should be removed @jorgecarleitao -- it might be how rust is run in the integration test framework.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

AFAIK this was only used to call Cargo fmt from Python; the build of the integration binaries is done by rust_build.sh; Python calls those binaries without Cargo.

I also checking the logs, we can find prints such as

##########################################################
IPC: C++ producing, Rust consuming
##########################################################

indicating that Rust is being tested.

@nevi-menevi-me left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@alamb

Copy link
Copy Markdown
Contributor

Ok, I think it is time to merge this PR @jorgecarleitao

@jorgecarleitao

Copy link
Copy Markdown
MemberAuthor

I think that we agreed over the mailing list to not merge this until @kszucs gives green light, post release.

I suggest that we take the time to complete the release, confirm that we are in a new development cycle, and then merge this. The rational being that I would like to reduce the risk of pushing merge conflicts to the release manager.

@kszucs

Copy link
Copy Markdown
Member

Yes, please hold off until we merge #10165.

@jorgecarleitao

Copy link
Copy Markdown
MemberAuthor

Rebased :)

Comment threaddev/archery/archery/cli.py Outdated
@kszucs

Copy link
Copy Markdown
Member

@jorgecarleitao@andygrove@alamb I think we can start to roll-up the rust pull requests now. There are a couple of open ones remaining, I assume we should close those and merge this one.

@kszucs

Copy link
Copy Markdown
Member

Rust releated part should be removed from ci/docker/linux-apt-lint.dockerfile.

There is a reference for the rust implementation in matlab/doc/matlab_interface_for_apache_arrow_design.md which
should be updated.

I assume the rust post release script should be removed as well ci/release/post-07-rust.sh since the new releases will be cut from arrow-rs.

There is a debian-rust configuration in the docker compose as well as in the crossbow tasks, but I guess we can keep that around to exercise the rust build script for the integration tests on a nightly basis.

@jorgecarleitao

Copy link
Copy Markdown
MemberAuthor

Thanks @kszucs . I have applied all those changes.

@alamb

alamb commented May 3, 2021

Copy link
Copy Markdown
Contributor

Thanks @jorgecarleitao

@kszucskszucs 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.

Thanks everyone! I'm merging it, hopefully we didn't forget anything!

@kszucskszucs closed this in 06c751bMay 3, 2021
@alamb

alamb commented May 3, 2021

Copy link
Copy Markdown
Contributor

Thank you @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

@jorgecarleitao@alamb@kszucs@andygrove@nevi-me