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

rustbuild: Migrate tidy checks to Rust #32590

Merged
merged 2 commits into from
Apr 13, 2016

Conversation

alexcrichton
Copy link
Member

This commit rewrites all of the tidy checks we have, namely:

  • featureck
  • errorck
  • tidy
  • binaries

into Rust under a new tidy tool inside of the src/tools directory. This at
the same time deletes all the corresponding Python tidy checks so we can be sure
to only have one source of truth for all the tidy checks.

cc #31590

@$(call E, check: feature sanity)
$(Q) $(CFG_PYTHON) $(S)src/etc/featureck.py $(S)src/

echo "make tidy" is now only available in rustbuild
Copy link
Contributor

Choose a reason for hiding this comment

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

So, if I use the old build system because testing is not implement in rustbuild yet, I won't be able to run make tidy anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, yeah, but if it's an important enough use case the tidy check is a small enough program to be easily compile-able

Copy link
Contributor

Choose a reason for hiding this comment

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

Forget about users, what about the buildbots? This disables tidy checking on Travis, which is unfortunate to say the least. Do we even gate on rustbuild bots yet, or will this allow "untidy" code to merge?

I see that this new tidy has no dependencies other than std so maybe it could still be compiled and run from the Makefiles with very little effort?

If that doesn't work, I would suggest at least making make tidy spit out an error, because this line is easily overlooked in the build log, and I know I will forget that it doesn't work anymore and run make tidy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we gate on rustbuild, no we won't check in "untidy" code because we're gating.

Yeah it's possible to just delete this rule altogether (to emit an error), but I figured we'd at least leave it here during a small transition period.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still sad to lose the quick feedback from Travis on trivial tidy issues. Please consider adding something to the effect of rustc src/tools/tidy/main.rs -o whereever/tidy && whereever/tidy.

Yeah just deleting the rule is bad because unexplained breakage, but echo bla bla && false would give both a message and an error. Though this does require adjusting .travis.yml and buildbots right now instead of soon-ish so I understand if you don't want to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, tidy errors slip through make check by default, Travis doesn't catch them as well because it uses the old build system too, so without extra effort from PR authors tidy errors will be detected only after PR approval by rustbuild buildbots and will need to go through re-approval(s). Seems bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still sad to lose the quick feedback from Travis on trivial tidy issues. Please consider adding something to the effect of rustc src/tools/tidy/main.rs -o whereever/tidy && whereever/tidy.

And it would be much better if this line is added to the old build system instead of Travis.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right so I figured no one actually runs make tidy and we don't really care much about it so long as it runs somewhere, and it's trivial to add a builder on Travis if we really want. If it's really wanted that much though then a rule can just be added

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@alexcrichton alexcrichton force-pushed the rustbuild-tidy-checks branch from 54d689f to e873e97 Compare March 30, 2016 05:07
@bors
Copy link
Contributor

bors commented Apr 2, 2016

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

@alexcrichton alexcrichton force-pushed the rustbuild-tidy-checks branch 4 times, most recently from db7c693 to e5a2b63 Compare April 5, 2016 18:21
@alexcrichton
Copy link
Member Author

Added a new tidy check which ensure that the dependencies listed in Cargo.toml are in sync with [dependencies]

@alexcrichton alexcrichton force-pushed the rustbuild-tidy-checks branch 2 times, most recently from 9d32f2e to 71ec190 Compare April 9, 2016 00:05
@brson
Copy link
Contributor

brson commented Apr 9, 2016

r=me but please squash the last commit

@alexcrichton alexcrichton force-pushed the rustbuild-tidy-checks branch from 71ec190 to ae70c39 Compare April 9, 2016 00:11
@alexcrichton
Copy link
Member Author

@bors: r=brson ae70c3913ab189407a15e8f6c25daa948708e85d

@bors
Copy link
Contributor

bors commented Apr 9, 2016

⌛ Testing commit ae70c39 with merge d9a3447...

@bors
Copy link
Contributor

bors commented Apr 9, 2016

💔 Test failed - auto-linux-64-x-freebsd

@bors
Copy link
Contributor

bors commented Apr 9, 2016

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

@alexcrichton alexcrichton force-pushed the rustbuild-tidy-checks branch from ae70c39 to a183085 Compare April 11, 2016 15:43
@alexcrichton
Copy link
Member Author

@bors: r=brson a183085

@bors
Copy link
Contributor

bors commented Apr 11, 2016

⌛ Testing commit a183085 with merge 403cf4c...

@bors
Copy link
Contributor

bors commented Apr 11, 2016

💔 Test failed - auto-linux-64-x-freebsd

@alexcrichton alexcrichton force-pushed the rustbuild-tidy-checks branch from a183085 to dfb28be Compare April 11, 2016 23:23
@alexcrichton
Copy link
Member Author

@bors: r=brson dfb28be

@bors
Copy link
Contributor

bors commented Apr 12, 2016

⌛ Testing commit dfb28be with merge 1ca6835...

@bors
Copy link
Contributor

bors commented Apr 12, 2016

💔 Test failed - auto-linux-32-nopt-t

@alexcrichton alexcrichton force-pushed the rustbuild-tidy-checks branch from dfb28be to 7ef35a8 Compare April 12, 2016 05:16
@alexcrichton
Copy link
Member Author

@bors: r=brson 7ef35a8

@bors
Copy link
Contributor

bors commented Apr 12, 2016

⌛ Testing commit 7ef35a8 with merge c765783...

@bors
Copy link
Contributor

bors commented Apr 12, 2016

💔 Test failed - auto-win-msvc-64-opt

This commit rewrites all of the tidy checks we have, namely:

* featureck
* errorck
* tidy
* binaries

into Rust under a new `tidy` tool inside of the `src/tools` directory. This at
the same time deletes all the corresponding Python tidy checks so we can be sure
to only have one source of truth for all the tidy checks.

cc rust-lang#31590
@alexcrichton alexcrichton force-pushed the rustbuild-tidy-checks branch from 7ef35a8 to 386ca6a Compare April 12, 2016 15:20
@alexcrichton
Copy link
Member Author

@bors: r=brson 386ca6a182ba27394dd0c84c0564d8c183d7525e

@bors
Copy link
Contributor

bors commented Apr 12, 2016

⌛ Testing commit 386ca6a with merge 07ee429...

@bors
Copy link
Contributor

bors commented Apr 12, 2016

💔 Test failed - auto-win-msvc-64-opt

This verifies that the crates listed in the `[dependencies]` section of
`Cargo.toml` are a subset of the crates listed in `lib.rs` for our in-tree
crates. This should help ensure that when we refactor crates over time we keep
these dependency lists in sync.
@alexcrichton alexcrichton force-pushed the rustbuild-tidy-checks branch from 386ca6a to 7bfaeaa Compare April 12, 2016 22:55
@alexcrichton
Copy link
Member Author

@bors: r=brson 7bfaeaa

@bors
Copy link
Contributor

bors commented Apr 12, 2016

⌛ Testing commit 7bfaeaa with merge a43eb4e...

bors added a commit that referenced this pull request Apr 12, 2016
rustbuild: Migrate tidy checks to Rust

This commit rewrites all of the tidy checks we have, namely:

* featureck
* errorck
* tidy
* binaries

into Rust under a new `tidy` tool inside of the `src/tools` directory. This at
the same time deletes all the corresponding Python tidy checks so we can be sure
to only have one source of truth for all the tidy checks.

cc #31590
@bors bors merged commit 7bfaeaa into rust-lang:master Apr 13, 2016
@alexcrichton alexcrichton deleted the rustbuild-tidy-checks branch April 13, 2016 23:48
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