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

Fix default arrow build #533

Merged
merged 2 commits into from
Jul 12, 2021
Merged

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jul 9, 2021

Which issue does this PR close?

Built on #532 so review that one first

Closes #529

Rationale for this change

Arrow does not build today unless rand 0.8 is included as another dependency of the project; I introduced this in #488 when I upgraded the rand dependency but disabled the default features (which require sys which mean arrow can't be built on webasm anymore)

Since the only use of the sys part of the rand dependency is in benchmarks and testing code, I propose simply not building that part of of the arrow crate if default features are turned off

What changes are included in this PR?

  1. Bring enough of the rand crate in by default to build, and #ifdef out their use when arrows default features are disabled

Are there any user-facing changes?

There is a test_utils feature flag now, enabled by default, that gates what parts of arrow are compiled if the default features are turned off

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jul 9, 2021
@alamb
Copy link
Contributor Author

alamb commented Jul 9, 2021

It would be nice to have all this test only code somewhere that wasn't the actual arrow crate at all, but since the test code has a backwards dependency on arrow -- (arrow <--> test_util) -- we would have to break that cycle as well, which is a lot of code churn

@alamb
Copy link
Contributor Author

alamb commented Jul 9, 2021

FYI @ritchie46 and @joshuataylor

@codecov-commenter
Copy link

Codecov Report

Merging #533 (abd8cfe) into master (f1fb2b1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #533   +/-   ##
=======================================
  Coverage   82.60%   82.60%           
=======================================
  Files         167      167           
  Lines       45984    45984           
=======================================
  Hits        37984    37984           
  Misses       8000     8000           
Impacted Files Coverage Δ
arrow/src/util/bit_util.rs 100.00% <ø> (ø)

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 f1fb2b1...abd8cfe. Read the comment docs.

@joshuataylor
Copy link

I have tested this by removing rand from my Cargo.toml and then changed arrow to this:

arrow = { git = "https://github.com/alamb/arrow-rs", branch = "alamb/fix-arrow-default-build" }

And everything works well. 👍

@alamb alamb force-pushed the alamb/fix-arrow-default-build branch from abd8cfe to 5f11c4f Compare July 12, 2021 15:08
@alamb alamb requested review from nevi-me and andygrove July 12, 2021 15:27
@alamb alamb merged commit 8d5a711 into apache:master Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

master fails to compile with default-features=false
4 participants