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

Infer targets from subdirectories #4496

Merged
merged 1 commit into from Sep 18, 2017

Conversation

Projects
None yet
4 participants
@rwakulszowa
Contributor

rwakulszowa commented Sep 15, 2017

Fixes #4086

I still have a few questions:

  • should I add some tests for the old behaviour? It isn't really tested at the moment (no tests failed when I broke the implementation); I could refactor the tests to check for both single file and subdirectory inference
  • I moved things around, mostly reusing the code from inferred_bins - hopefully I didn't break anything, but it won't hurt to double check :)
  • Do we have something like servo's tidy check for coding style? I'm open for suggestions if something isn't formatted correctly
  • Just a general one - should I rebase + squash commits every time I make subsequent changes to cargo?
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Sep 15, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (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.

rust-highfive commented Sep 15, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (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.

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Sep 17, 2017

Member

should I add some tests for the old behaviour? It isn't really tested at the moment (no tests failed when I broke the implementation); I could refactor the tests to check for both single file and subdirectory inference

Adding test for the old behavior is very important, because backwards compatibility is one of the major constraints of Cargo. Refactoring tests to check for all flavors of conventions seems like a great idea!

Do we have something like servo's tidy check for coding style? I'm open for suggestions if something isn't formatted correctly

The only hard rule we have is that line length should be under 100 (it's checked on CI). Otherwise, it's a good idea to stick to an existing code style (which unfortunately may vary depending on which file you are editing now). We are not ready to use rustfmt yet.

Just a general one - should I rebase + squash commits every time I make subsequent changes to cargo?

It does not matter a lot, but an ideal appraoch I think is to do rebasing/squshing and other history rewrites, if necessary, and then ping the reviewer by the comment, because GH does not deliver notifications about force pushes.

I moved things around, mostly reusing the code from inferred_bins - hopefully I didn't break anything, but it won't hurt to double check :)

Will take a look shortly! In general, I feel rather confident about our tests :)

Member

matklad commented Sep 17, 2017

should I add some tests for the old behaviour? It isn't really tested at the moment (no tests failed when I broke the implementation); I could refactor the tests to check for both single file and subdirectory inference

Adding test for the old behavior is very important, because backwards compatibility is one of the major constraints of Cargo. Refactoring tests to check for all flavors of conventions seems like a great idea!

Do we have something like servo's tidy check for coding style? I'm open for suggestions if something isn't formatted correctly

The only hard rule we have is that line length should be under 100 (it's checked on CI). Otherwise, it's a good idea to stick to an existing code style (which unfortunately may vary depending on which file you are editing now). We are not ready to use rustfmt yet.

Just a general one - should I rebase + squash commits every time I make subsequent changes to cargo?

It does not matter a lot, but an ideal appraoch I think is to do rebasing/squshing and other history rewrites, if necessary, and then ping the reviewer by the comment, because GH does not deliver notifications about force pushes.

I moved things around, mostly reusing the code from inferred_bins - hopefully I didn't break anything, but it won't hurt to double check :)

Will take a look shortly! In general, I feel rather confident about our tests :)

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Sep 17, 2017

Member

Changes look good to me! Perhaps we should get rid of inferred_(examples|benches|tests) one-liners and call infer_from_directory directly?

Also we need docs for this at https://github.com/rwakulszowa/cargo/blob/master/src/doc/manifest.md#the-project-layout.

And more tests are always welcome, especially if they test long-preexisting, but not currently covered functionality :)

Member

matklad commented Sep 17, 2017

Changes look good to me! Perhaps we should get rid of inferred_(examples|benches|tests) one-liners and call infer_from_directory directly?

Also we need docs for this at https://github.com/rwakulszowa/cargo/blob/master/src/doc/manifest.md#the-project-layout.

And more tests are always welcome, especially if they test long-preexisting, but not currently covered functionality :)

@matklad matklad added the relnotes label Sep 17, 2017

@rwakulszowa

This comment has been minimized.

Show comment
Hide comment
@rwakulszowa

rwakulszowa Sep 17, 2017

Contributor

I'm using the cargo test --test bar --test baz to check for existence of multiple tests currently. It seems that the logic is different for benchmarks - cargo bench allows me to pass only a single --bench=<name> argument (i.e.

cargo bench --bench=a --bench=b

throws an exception. Is this the expected behavior or is it a bug?

Contributor

rwakulszowa commented Sep 17, 2017

I'm using the cargo test --test bar --test baz to check for existence of multiple tests currently. It seems that the logic is different for benchmarks - cargo bench allows me to pass only a single --bench=<name> argument (i.e.

cargo bench --bench=a --bench=b

throws an exception. Is this the expected behavior or is it a bug?

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Sep 17, 2017

Member

I would think it's a bug

Member

matklad commented Sep 17, 2017

I would think it's a bug

@rwakulszowa

This comment has been minimized.

Show comment
Hide comment
@rwakulszowa

rwakulszowa Sep 18, 2017

Contributor

I've added the requested changes. I've also created an issue for multiple --bench arguments (#4504) and marked the affected code with a #FIXME.

I also think I could clean up the tests for bin a little bit - don't you think this test already covers all cases for bin inference, so this one can be removed? Also, doesn't it have some duplicate assertions?

One more thing - currently my fake bench and test files only have a fn main(...) - is this good enough for these tests, since they are only supposed to test the build system, while benchmark specific tests are in bench.rs, or should I add some simple, but real code? This would complicate things a bit for benchmarks, since they only work with nightly cargo afaik.

Contributor

rwakulszowa commented Sep 18, 2017

I've added the requested changes. I've also created an issue for multiple --bench arguments (#4504) and marked the affected code with a #FIXME.

I also think I could clean up the tests for bin a little bit - don't you think this test already covers all cases for bin inference, so this one can be removed? Also, doesn't it have some duplicate assertions?

One more thing - currently my fake bench and test files only have a fn main(...) - is this good enough for these tests, since they are only supposed to test the build system, while benchmark specific tests are in bench.rs, or should I add some simple, but real code? This would complicate things a bit for benchmarks, since they only work with nightly cargo afaik.

@rwakulszowa rwakulszowa changed the title from Infer targets from subdirectories - WIP to Infer targets from subdirectories Sep 18, 2017

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Sep 18, 2017

Member

don't you think this test already covers all cases for bin inference, so this one can be removed?

I don't think so. The second test is different in that it has an explicit [[bin]] section in Cargo.toml, so it tests that we can map from a binary name to its path corretly. It would be fine to merge these tests into one though.

Also, doesn't it have some duplicate assertions?

Yeah, some assetions indeed are twins.

One more thing - currently my fake bench and test files only have a fn main(...) - is this good enough for these tests, since they are only supposed to test the build system, while benchmark specific tests are in bench.rs, or should I add some simple, but real code?

It's ok to use fake code.

Looks like you've studied the relevant tests deeply, so, if you think you can organize them better than they are orginized currently, go ahead :) The current situation is pretty much historical, and I expect that the test suite can be made shorter and easier to undestand without compromizing test coverage.

So let's r+ this right away, and you can create a separate PR for tests cleanup then!

@bors r+

Thanks, @rwakulszowa !

Member

matklad commented Sep 18, 2017

don't you think this test already covers all cases for bin inference, so this one can be removed?

I don't think so. The second test is different in that it has an explicit [[bin]] section in Cargo.toml, so it tests that we can map from a binary name to its path corretly. It would be fine to merge these tests into one though.

Also, doesn't it have some duplicate assertions?

Yeah, some assetions indeed are twins.

One more thing - currently my fake bench and test files only have a fn main(...) - is this good enough for these tests, since they are only supposed to test the build system, while benchmark specific tests are in bench.rs, or should I add some simple, but real code?

It's ok to use fake code.

Looks like you've studied the relevant tests deeply, so, if you think you can organize them better than they are orginized currently, go ahead :) The current situation is pretty much historical, and I expect that the test suite can be made shorter and easier to undestand without compromizing test coverage.

So let's r+ this right away, and you can create a separate PR for tests cleanup then!

@bors r+

Thanks, @rwakulszowa !

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 18, 2017

Contributor

📌 Commit 6b94ed2 has been approved by matklad

Contributor

bors commented Sep 18, 2017

📌 Commit 6b94ed2 has been approved by matklad

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 18, 2017

Contributor

⌛️ Testing commit 6b94ed2 with merge 7619c50...

Contributor

bors commented Sep 18, 2017

⌛️ Testing commit 6b94ed2 with merge 7619c50...

bors added a commit that referenced this pull request Sep 18, 2017

Auto merge of #4496 - rwakulszowa:infer_from_subdirectories, r=matklad
Infer targets from subdirectories

Fixes #4086

I still have a few questions:
- should I add some tests for the old behaviour? It isn't really tested at the moment (no tests failed when I broke the implementation); I could refactor the tests to check for both single file and subdirectory inference
- I moved things around, mostly reusing the code from `inferred_bins` - hopefully I didn't break anything, but it won't hurt to double check :)
- Do we have something like servo's `tidy` check for coding style? I'm open for suggestions if something isn't formatted correctly
- Just a general one - should I rebase + squash commits every time I make subsequent changes to cargo?
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 18, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 7619c50 to master...

Contributor

bors commented Sep 18, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 7619c50 to master...

@bors bors merged commit 6b94ed2 into rust-lang:master Sep 18, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@rwakulszowa rwakulszowa deleted the rwakulszowa:infer_from_subdirectories branch Sep 18, 2017

kumbayo added a commit to kumbayo/intellij-rust that referenced this pull request Oct 4, 2017

CARGO: Support main.rs as crate root in various subfolder
This feature was added after cargo 0.21
(not released with any stable Rust version yet)
rust-lang/cargo#4496

bors added a commit that referenced this pull request Oct 23, 2017

Auto merge of #4654 - rwakulszowa:inferred_test_cleanup, r=matklad
Clean up inferred* tests

Deduplicate, simplify and rename interred_* tests.
Follow up to #4496
@matklad

bors added a commit that referenced this pull request Oct 25, 2017

Auto merge of #4654 - rwakulszowa:inferred_test_cleanup, r=matklad
Clean up inferred* tests

Deduplicate, simplify and rename interred_* tests.
Follow up to #4496
@matklad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment