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 a caveats section for conditional compilation #1195

Closed
wants to merge 5 commits into from
Closed

Add a caveats section for conditional compilation #1195

wants to merge 5 commits into from

Conversation

yoav-lavi
Copy link

This section explains the caveats and possible solutions for conditional compilation scenarios, detailing the configuration and tooling needed to properly cover such a project.

The need for this PR is based on a conversation with Eh2406

This section explains the caveats and possible solutions for conditional compilation scenarios, detailing the configuration and tooling needed to properly cover such a project.

The need for this PR is based on a [conversation with Eh2406](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/build.20variants/near/278972904)
@yoav-lavi
Copy link
Author

@bjorn3 tagging you since I can't find Eh2406's user, please let me know if I should tag someone else. Thanks!

@bjorn3
Copy link
Member

bjorn3 commented Apr 19, 2022

cc @ehuss

@ehuss
Copy link
Contributor

ehuss commented Apr 20, 2022

Thanks for the PR! We generally prefer to not have guide-style material in the reference. We also don't want to link to third-party tools here. Unfortunately we don't have official guides for intermediate and advanced topics (things such as this, advanced testing, macros, etc.). So unfortunately I don't have a good suggestion on where advice like this can live.

@yoav-lavi
Copy link
Author

Thanks @ehuss, I understand, would the cargo feature docs be a good fit?

cc @Eh2406

@yoav-lavi
Copy link
Author

I initially made the change here since this is the place I first checked when looking into this

@ehuss
Copy link
Contributor

ehuss commented Apr 21, 2022

I think it might be reasonable to have a small section in cargo's features chapter. I think it would need to be reworded somewhat, though. I don't think it should make specific suggestions on how to test the factorial combinations of configurations, or make the assertion what is "proper". Every project will have different needs and resources. We also shouldn't suggest any particular third-party tooling. I would just include a few sentences to remind the reader that a package may be built with any combination of features, and recommend that they consider how they will want to test those combinations.

@yoav-lavi
Copy link
Author

yoav-lavi commented Apr 22, 2022

@ehuss Adding the external tools was based on this comment by @Eh2406:

wordsmithing aside: acknowledgment that thoroughly testing the interactions of features and platforms requires an exponential number of test cases, and that there are tools that help you generate them seems like a comment worth adding.

This followed a conversation that discussed adding this functionality to cargo. While the general consensus was that this would be beneficial, it wasn't deemed mature enough so they suggested to link to the existing solutions.

I completely understand where you're coming from, my concern is that there's a certain catch-22 here if functionality isn't added + existing tools aren't mentioned, there basically is no "official" way or guidance on how to handle this, while it's inherent to how Rust works (which makes sense, I'm only saying that I think that an official method / recommendation of handling this also makes sense)

Of course I'm new to contributing to Rust and I may be missing something, I don't mean to state a fact, just to raise my concerns.

Regarding the word proper: I could remove it, the intent is "to properly cover all cases ..." but it can be reworded.

To clarify: I'm not affiliated with the suggested crates in any way, my intention is to supply a specific method of doing this

@Eh2406
Copy link

Eh2406 commented Apr 25, 2022

First off, thank you for making a PR for this!

I do think Eric is correct, and the language reference is definitely the wrong fit for this content. (Not just on style grounds, another implementation of Rust may have a different compilation model with different implications.)

With some care I think it can fit with the content in cargos reference https://github.com/rust-lang/cargo/blob/master/src/doc/src/reference/features.md Speaking of care, all official documentation on behalf of the Cargo Team get looked over with a fine tooth comb. Wordsmithing is hard and important work. Don't take it personally if it takes a few rounds of editing to find the balance among different concerns.

my concern is that there's a certain catch-22 here

You are not wrong. Where there are acknowledged problems without clear solutions official statements end up in a kind of limbo. If there was a solution, it would be made first class and get thoroughly documented. If there was no problem, clear up the confusion and move on. In the in between cases we need to be extremely careful. Endorsing any particular prototype solution can (and has) led to stultifying the ecosystem preventing the adoption/development of new better solutions. Not acknowledging the problem has its own obvious costs, in addition to preventing people from realizing it's a problem where new development would be helpful.

@yoav-lavi
Copy link
Author

yoav-lavi commented Apr 27, 2022

Thanks for the response @Eh2406! Will move this PR to that documentation.

@yoav-lavi yoav-lavi closed this Apr 27, 2022
bors added a commit to rust-lang/cargo that referenced this pull request May 5, 2022
Add caveat for covering features

This section explains the caveats and possible solutions for features, detailing the tooling needed to 100% cover such a project.

The need for this PR is based on a conversation with `@Eh2406.`

Moved this PR here based on `@Eh2406's` comment: rust-lang/reference#1195 (comment)
Hezuikn pushed a commit to Hezuikn/cargo that referenced this pull request Sep 22, 2022
This section explains the caveats and possible solutions for features, detailing the tooling needed to 100% cover such a project.

The need for this PR is based on a conversation with @Eh2406.

Moved this PR here based on @Eh2406's comment: rust-lang/reference#1195 (comment)
Hezuikn pushed a commit to Hezuikn/cargo that referenced this pull request Sep 22, 2022
This section explains the caveats and possible solutions for features, detailing the tooling needed to 100% cover such a project.

The need for this PR is based on a conversation with @Eh2406.

Moved this PR here based on @Eh2406's comment: rust-lang/reference#1195 (comment)
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.

4 participants