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

Make CI build & test use default and all feature sets #121

Merged
merged 1 commit into from
Jun 12, 2020

Conversation

echeran
Copy link
Contributor

@echeran echeran commented Jun 11, 2020

This is to allow both the build and test commands in cargo to run both with default features enabled and with --all-features.

I was not able to use --no-default-features:

$ cargo build --no-default-features
error: --no-default-features is not allowed in the root of a virtual workspace
note: while this was previously accepted, it didn't actually do anything
$ cargo test --no-default-features
error: --no-default-features is not allowed in the root of a virtual workspace
note: while this was previously accepted, it didn't actually do anything

A test run shows the locale component's serde unit tests running when all features (including the serde feature) are enabled, and they don't run in the default setting, as expected.

Is the lack of --no-default-features okay for this PR, or should I continue looking into cargo-all-features as @zbraniecki mentioned in order to address the scenario of testing with std disabled?

@filmil
Copy link
Contributor

filmil commented Jun 11, 2020

You could make a cargo manifest non-virtual, in which case I think cargo will allow you to do this.

@coveralls
Copy link

Pull Request Test Coverage Report for Build a25634d21fd1555074bc1a501a904e7a1a2b8fe3-PR-121

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 87.199%

Totals Coverage Status
Change from base Build a0d0cfec033b6db6a500c927cca7bc7e5beff280: 0.0%
Covered Lines: 688
Relevant Lines: 789

💛 - Coveralls

@zbraniecki
Copy link
Member

I'm ok with this PR. We can add test-all-features later.

@echeran echeran requested review from filmil, sffc and zbraniecki June 12, 2020 01:09
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

An improvement for now, and hopefully we can figure out how to get the no-feature build working as well.

@sffc
Copy link
Member

sffc commented Jun 12, 2020

Main issue: #63

@echeran echeran merged commit 6d9dfdd into unicode-org:master Jun 12, 2020
zbraniecki pushed a commit to zbraniecki/icu4x that referenced this pull request Jun 18, 2020
zbraniecki pushed a commit to zbraniecki/icu4x that referenced this pull request Jun 18, 2020
zbraniecki pushed a commit to zbraniecki/icu4x that referenced this pull request Jun 18, 2020
@echeran echeran deleted the ci-feature-sets branch October 6, 2020 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants