Checking for binary that is built as an implicit dependency of an integration test.

[xiongmao86]

When a project containing binary with required-features, the binary will only be built with the given dependency's feature is enabled. But this feature syntax is not checked when the binary is built as an implicit dependency of an integration test. This pr addresses this issue.

More info is in the issue page: this pr fixes #7853.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Oh wow I'm actually a bit surprised we allowed this in required-features! In any case mind leaving a comment in the code explaining what it's doing? Otherwise looks good to me, thanks for the PR!

[ehuss]

Oh, and just a heads up, resolve_all_features may need to change as part of #7950, so we should be careful with coordinating these two changes at the same time.

[xiongmao86]

@alexcrichton @ehuss, thank you for reviewing the pr.

In any case mind leaving a comment in the code explaining what it's doing?

I'll try to add comment as the last step.

[xiongmao86]

Oh, and just a heads up, resolve_all_features may need to change as part of #7950, so we should be careful with coordinating these two changes at the same time.

What do you mean by coordinating these two changes? Can you elaborate?

[aleksator]

What do you mean by coordinating these two changes? Can you elaborate?

I think that the PR which is going to be merged second might have to deal with some conflicts.
Since yours looks almost done, it would have to be me in 7950, so you have nothing to worry about.

[xiongmao86]

Oh, right. Thank you for explaining @aleksator.

[bors]

☔ The latest upstream changes (presumably #8028) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

Looks good, just one minor nit!

[aleksator]

@xiongmao86 Also, don't forget to squash the history ;)

[aleksator]

@xiongmao86 I'd put tests in the same commit as changes they are supposed to test as well.
But I'm not a maintainer here and don't know how the people prefer it in this project, just what I would consider a good practice.

[ehuss]

Thanks!
@bors r+

[bors]

📌 Commit 8b17895 has been approved by ehuss

[bors]

⌛ Testing commit 8b17895 with merge 1e2f8db...

[bors]

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 1e2f8db to master...

[aleksator]

Congrats on the first successful PR!

[xiongmao86]

@alexcrichton, @ehuss, @aleksator, thank you very much for helping me with this pr.

I'd put tests in the same commit as changes they are supposed to test as well.
But I'm not a maintainer here and don't know how the people prefer it in this project, just what I would consider a good practice.

Perhaps because of the three phase of writing code in agile development: test, implement, refactor. Here refactor is mix with implement because of squash?

[Eh2406]

I'd put tests in the same commit as changes they are supposed to test as well.

That does sound like a good ideal, but as a maintainer of this project we do not require a clean history in a PR. If you want to rewrite history so each commit makes sense on its own, in whatever way makes you happy, grate! If you what to record the order you actually made changes, with all of the wrong turns, also grate!

[xiongmao86]

Thank you for your tips, @Eh2406, so I am free to choose squash or not. But some repo does need to squash, what is the possible reason, is it possible in some specific situation that one approach is preferable than the other?

[Eh2406]

Some people/projects really value clean historys, or the ability to bisect to an individual commit. There going to tend to want edited historys, or if that is to much work squashing. Other project put there priorities on different things.

[xiongmao86]

So it's a delicate tradeoff, and depend on project and people. Thank you @Eh2406.

[aleksator]

@xiongmao86 In my opinion, it's always good to have, but forcing it requires some amount of work, which for open source project might not be worth it and can turn off contributors if applied too rigidly. That's why cargo chooses not to (I think).

At my workplace we choose to do so, as it has clear benefits and it's easy to have discipline within a small team, but much harder with potentially all of the internet for open source projects.

For me it's easier to understand a feature that way, when the commit is self contained. You don't have to waste time tracking down what is this test supposed to check, when you can just click on a line annotation in an IDE and be shown connected feature right there. And vise versa, when you make changes you know which tests you will need to fix.

Perhaps because of the three phase of writing code in agile development: test, implement, refactor. Here refactor is mix with implement because of squash?

As you are developing a new feature, from the point of project's view refactoring does not exist yet. You should squash it and hide your work in progress, as it is only harmful noise.
It's a good idea to make a local branch with full history backup in case something goes wrong with the rebase, but it's not a master branch material.

That said, when you change the existing code, that is refactoring that's important to show others. It should never be mixed into your new feature commit, but always provided separately.
This way you can easily track down bugs if something is broken: was it due to the refactoring or a new feature addition. If you are in a hurry, you can revert just that one bit with one click.

[xiongmao86]

@aleksator, I see your point, in this case the test is good to commit with the feature. but sometimes the change is too big to squash together, as what the test account for is separate into several commits, then it is hard or sometimes impossible(if what I add is a single test) to separate the test, this is a time I feel a test is stand out as a single commit content. So is it ok to do so in this case, or there is actually a better way to divide the changes?

[xiongmao86]

I have read about the concept of incremental development that is we should take small steps to change code base, in this prospect, will large changes inherently bad to show up? Would this be the answer to my doubts above?

[Eh2406]

I have not worked in a "you must squash all PRs" codebase, and it seems a extreme position to me. But to each their own. I have contributed to "commits should be incremental and interpretable" codebase. If a PR comes in that took a lot of commits to do something straightforward, squashing can be an improvement. If a big PR comes in, to big to understand in one sitting, asking that the contributor ( who probably, just having written it, understand it better than anyone ) to split it up into commits/PRs that can be reviewed one at a time, can also be reasonable.

Just like how splitting your code into functions can make it easier to read. Does not mean that more functions is always better. You as a programer need to use functions to communicate what you were trying to do. Git/PRs/commits are the same way. Use the tools you have in moderation and with judgment to make your intent intelligible to others.

[xiongmao86]

@aleksator, @Eh2406, I see your point. Thank you very much for explaining.

[aleksator]

@Eh2406 Our views aren't different, actually. I'd do the same in the situation you described.