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

Provide an ergonomic way to check code with all features #1337

Closed
ghost opened this issue Dec 9, 2019 · 4 comments
Closed

Provide an ergonomic way to check code with all features #1337

ghost opened this issue Dec 9, 2019 · 4 comments
Labels
domain: ci Anything related to Vector's CI environment domain: tests Anything related to Vector's internal tests type: task Generic non-code related tasks

Comments

@ghost
Copy link

ghost commented Dec 9, 2019

Before #1331 make check was equivalent to cargo check --all --all-features --all-targets.

However, after #1331 the feature pairs leveldb-plain/leveldb-cmake and rdkafka-plain/rdkafka-cmake became mutually exclusive. These pairs of mutually exclusive features needed to ensure that all dependencies for leveldb and rdkafka are correctly written to Cargo.lock, so that Vector can be built with --freeze build flag. Cargo doesn't support defining mutually exclusive features yet (see rust-lang/cargo#2980), so the command in make check was changed to cargo check --all --all-targets.

It means that docker feature is not checked anymore when make check is called, although it is still compiled in CI as part of test-stable.

One approach is to change check-code target in the Makefile to something like

@cargo check --all  --all-targets
@cargo check --all  --features=nightly --all-targets
@cargo check --all  --features=docker --all-targets

However, this approach doesn't look like very elegant and there might be a better solution. I'd like to hear @LucioFranco's thoughts about this.

@ghost ghost added domain: tests Anything related to Vector's internal tests domain: operations labels Dec 9, 2019
@LucioFranco
Copy link
Contributor

So a couple of things, I am curious if there is a solution that doesn't use mutually exclusive feature flags. This in of it self breaks kinda how cargo handles feature flags as you can tell from the issue you linked. For me it's a bit important to not use make as I am using rust-analyzer which then uses cargo watch thus I would like to make sure we keep a workflow that is workable through cargo.

In the end, if the only feature flag that I need to add beyond default is docker, I think it is fine. But I'm not a big fan of having a huge array of feature flags and combinations which can lead us to missing things sometimes.

@ghost
Copy link
Author

ghost commented Dec 13, 2019

I was thinking about this, it could be fixed by adding support for building new versions of LevelDB in leveldb-sys create on GNU/Linux and macOS without cmake, but using only cc crate.

It would allow us to keep features for leveldb crate simple, just leveldb and leveldb,leveldb/cmake_build, so that two types of the builds would be not mutually exclusive.

@LucioFranco
Copy link
Contributor

Seems like a good idea.

@binarylogic binarylogic added domain: ci Anything related to Vector's CI environment and removed domain: operations labels Jul 29, 2020
@binarylogic binarylogic added the type: task Generic non-code related tasks label Aug 7, 2020
@jszwedko
Copy link
Member

jszwedko commented Aug 1, 2022

Closing this since it hasn't seemed to be an issue.

@jszwedko jszwedko closed this as completed Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: ci Anything related to Vector's CI environment domain: tests Anything related to Vector's internal tests type: task Generic non-code related tasks
Projects
None yet
Development

No branches or pull requests

3 participants