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

Add tests for building applications using arrow with different feature flags #532

Merged
merged 2 commits into from
Jul 12, 2021

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jul 9, 2021

Which issue does this PR close?

Re: #529

Rationale for this change

The current CI test (here) to ensure arrow builds with various feature flags did not catch #529. This is because it is run in the context of the overall workspace which does not behave the same way as when arrow is used as a dependency

This PR contains the test strategy I propose to use for testing that arrow properly builds with different options. I plan a follow on PR that actually fixed #529

What changes are included in this PR?

This PR contains the test strategy I propose to use for testing that arrow properly builds with different options. I plan a follow on PR that actually fixed #529

Changes

  1. Create actual projects that specify arrow as a dependency with various flags
  2. Try to build that project in CI

Here is an example of the test running on CI: https://github.com/apache/arrow-rs/runs/3032269357?check_suite_focus=true (it is quite fast to check the projects)

You easily run these builds locally:

cd arrow/test/dependency/no-features
cargo check

I am sure there is some clever tool somewhere that claims to solve this problem (perhaps cargo hack? as suggested by @ritchie46). However, I chose to do it explicitly here because:

  1. It is as close as possible to how an actual user will build their app with arrow
  2. I can audit how it works without having to review projects and asses how mature / stable they are

Are there any user-facing changes?

no

@alamb alamb marked this pull request as draft July 9, 2021 19:45
@github-actions github-actions bot added the arrow Changes to the arrow crate label Jul 9, 2021
@alamb alamb force-pushed the alamb/arrow-build-flag-tests branch from d69b6b1 to a9a0728 Compare July 9, 2021 19:51
arrow = { path = "../../../../arrow", version = "5.0.0-SNAPSHOT" }

# Workaround for https://github.com/apache/arrow-rs/issues/529
rand = { version = "0.8" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When this line is removed the test fails. I will remove it with the fix for #529

@codecov-commenter
Copy link

Codecov Report

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

❗ Current head c1b5fa6 differs from pull request most recent head 65db5ad. Consider uploading reports for the commit 65db5ad to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #532   +/-   ##
=======================================
  Coverage   82.60%   82.60%           
=======================================
  Files         167      167           
  Lines       45984    45984           
=======================================
  Hits        37984    37984           
  Misses       8000     8000           

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...65db5ad. Read the comment docs.

@alamb alamb mentioned this pull request Jul 9, 2021
@alamb alamb marked this pull request as ready for review July 9, 2021 20:39
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.

I think this strategy is sound, LGTM

# Workaround for https://github.com/apache/arrow-rs/issues/529
rand = { version = "0.8" }

[workspace]
Copy link
Contributor

Choose a reason for hiding this comment

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

stray line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this is required (you get a cargo build error without it). Somehow this signals to cargo that this project is not part of the workspace rooted at arrow-rs/Cargo.toml

run: |
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd arrow
cargo check --all-targets --no-default-features
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be what was missing ito catching other feature types? That we weren't running cargo check --all-targets --features simd,...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the reason CI isn't catching issues with feature flags is due to the cargo semantics of a workspace -- where Cargo tries to calculate the dependency needs of the workspace as a whole. Since we have some crates in the workspace (like arrow-flight) that bring in additional dependencies, these additional dependencies can obscure build problems with other crates (like arrow) when used alone

@alamb alamb merged commit 787e826 into apache:master Jul 12, 2021
@alamb alamb deleted the alamb/arrow-build-flag-tests branch July 12, 2021 16:05
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
3 participants