Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-12444: [Rust] Remove rust #10096

Closed
wants to merge 13 commits into from
Closed

ARROW-12444: [Rust] Remove rust #10096

wants to merge 13 commits into from

Conversation

jorgecarleitao
Copy link
Member

@jorgecarleitao jorgecarleitao commented Apr 19, 2021

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 link
Contributor

@alamb alamb left a comment

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)

@@ -1,23 +0,0 @@
# Licensed to the Apache Software Foundation (ASF) under one
Copy link
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
Member Author

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.

Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

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

LGTM

@alamb
Copy link
Contributor

alamb commented Apr 26, 2021

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

@jorgecarleitao
Copy link
Member Author

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
Member

kszucs commented Apr 26, 2021

Yes, please hold off until we merge #10165.

@jorgecarleitao
Copy link
Member Author

Rebased :)

@kszucs
Copy link
Member

kszucs commented Apr 30, 2021

@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
Member

kszucs commented Apr 30, 2021

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 jorgecarleitao reopened this May 2, 2021
@jorgecarleitao
Copy link
Member Author

Thanks @kszucs . I have applied all those changes.

@alamb
Copy link
Contributor

alamb commented May 3, 2021

Thanks @jorgecarleitao

Copy link
Member

@kszucs kszucs left a comment

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!

@kszucs kszucs closed this in 06c751b May 3, 2021
@alamb
Copy link
Contributor

alamb commented May 3, 2021

Thank you @kszucs

DileepSrigiri pushed a commit to DileepSrigiri/arrow that referenced this pull request May 6, 2021
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:
* https://github.com/apache/arrow-rs
* https://github.com/apache/arrow-datafusion

Closes apache#10096 from jorgecarleitao/rm-rust

Authored-by: Jorge C. Leitao <[email protected]>
Signed-off-by: Krisztián Szűcs <[email protected]>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 10, 2021
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:
* https://github.com/apache/arrow-rs
* https://github.com/apache/arrow-datafusion

Closes apache#10096 from jorgecarleitao/rm-rust

Authored-by: Jorge C. Leitao <[email protected]>
Signed-off-by: Krisztián Szűcs <[email protected]>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 2021
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:
* https://github.com/apache/arrow-rs
* https://github.com/apache/arrow-datafusion

Closes apache#10096 from jorgecarleitao/rm-rust

Authored-by: Jorge C. Leitao <[email protected]>
Signed-off-by: Krisztián Szűcs <[email protected]>
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.

5 participants