-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
cargo check #3296
cargo check #3296
Conversation
One thing I want to do: check.rs is a copy and paste version of build.rs with some renaming. I would like to share some code. Is it OK to have non-command modules in src/bin? |
@@ -51,6 +51,7 @@ Options: | |||
|
|||
Some common cargo commands are (see all commands with --list): | |||
build Compile the current project | |||
check Check the current project without compiling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description is weaker because it re-uses 'check'. And it does compile, in a sense....
I'll try to come up with a suggestion for better words later.
@@ -72,7 +72,7 @@ impl LibKind { | |||
"lib" => LibKind::Lib, | |||
"rlib" => LibKind::Rlib, | |||
"dylib" => LibKind::Dylib, | |||
"procc-macro" => LibKind::ProcMacro, | |||
"proc-macro" => LibKind::ProcMacro, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this related to this new feature or some kind of other random bugfix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
random bugfix
@@ -0,0 +1,132 @@ | |||
.TH "CARGO\-BUILD" "1" "May 2016" "The Rust package manager" "Cargo Manual" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lots of cargo build in this file still
@@ -649,6 +671,12 @@ impl<'a, 'cfg> Context<'a, 'cfg> { | |||
}) | |||
} | |||
|
|||
pub fn metadata_crate(&self, unit: &Unit<'a>) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "metadata" name is a bit unfortunate, because there are already two (different) metadatas in Cargo already: the one in package_id.rs
and another in bin/metadata.rs
. But looks like this is some compiler flag, so we can't really change it :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited to see progress!
I think that we may want not want to store check as a global mode (e.g. self.build_config.check
) but rather as a property of the Unit
. That is, sort of like build scripts, we'd transitively include a Target
which think it's a "check" target. With that we can then correctly calculate dependencies and such in a transitive fashion. For example, the rules would look like:
- When we check a target, we transitively check dependencies.
- When checking a target, we still need to compile and run the build script
- When checking a target, we need to compile proc-macro crates as objects and such
This I think will be much easier to represent as a property of the target itself rather than a global mode.
One tricky thing we'll also need to handle is a case such as this. You've got a crate A which depends on B as both a dependency and a build dependency. That means when you cargo check
A you'll have to actually compile the object code for B (it's a build dependency). We shouldn't also run check over it though.
--features FEATURES Space-separated list of features to also check | ||
--all-features Build all available features | ||
--no-default-features Do not check the `default` feature | ||
--target TRIPLE Build for the target triple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll probably want a general s/Build/Check/ in this file
@@ -127,6 +137,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> { | |||
// `lib` in the compiler and we're not sure which we'll see. | |||
crate_types.insert("bin".to_string()); | |||
crate_types.insert("rlib".to_string()); | |||
// TODO comment | |||
crate_types.insert("metadata".to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failures on stable (via Travis) point to we'll need to gracefully handle the situation where the compiler we're running against doesn't support --crate-type metadata
.
dd2767b
to
fd888ac
Compare
So, I think we satisfy these rules already. Cargo has the transitive dep info already, so it seemed wrong to re-compute it. So, if we are in global 'check' mode, when we come to build a crate, we check if it is transitively required for a running crate (proc macro or build script) and if so, we build it rather than check it. It didn't seem useful to store that info on each
I'll have to double check, but I believe we handle this case OK. |
Yep, the current code does the correct thing |
Ah sorry I missed the check for This looks, though, that this definitely has the feeling of a "bolted on" backend rather than integrated with the |
Pushed a commit with the approach @alexcrichton and I discussed. Still not finished, but would be great to get another round of feedback. |
@@ -788,6 +811,10 @@ impl<'a, 'cfg> Context<'a, 'cfg> { | |||
} | |||
} | |||
|
|||
pub fn check_profile(&self, _pkg: &PackageId) -> &'a Profile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to avoid a new method here, lib_profile
was added a point I thought I would use it but never ended up doing so.
@@ -173,6 +173,15 @@ fn compile<'a, 'cfg: 'a>(cx: &mut Context<'a, 'cfg>, | |||
return Ok(()) | |||
} | |||
|
|||
// REVIEW moving this code changes the dependency traversal from pre- to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be ok, yeah.
Although I'd prefer for it to not be necessary, I'll comment below.
let mut lib_unit = unit.clone(); | ||
lib_unit.profile = cx.lib_profile(lib_unit.pkg.package_id()); | ||
let (freshness, lib_dirty, lib_fresh) = fingerprint::prepare_target(cx, &lib_unit)?; | ||
if freshness == Freshness::Fresh { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So historically the fingerprint
module only checked whether one target was up to date, not checking dependencies. If, however, a dependency is recompiled we'll end up wanting to recompile this target even though the dependency information says it's fresh (as a dep changed).
I forget if fingerprint
has since changed to take into account dependencies automatically, or whether that's still a concern of the DependencyQueue
. Could you verify this?
In any case, the expected behavior for me would be that if you're running cargo check
and there's an "fresh rlib" but it ends up needing recompiling anyway due to some other dep, we'd want to generate the rmeta instead of the rlib here.
Updated and ready for review. |
Awesome! r+ from me Gonna work on Cargo's CI for a bit, but I'll r+ when done |
@bors: r+ |
📌 Commit d49d52d has been approved by |
⌛ Testing commit d49d52d with merge e260560... |
💔 Test failed - status-travis |
@bors: retry hmm |
⌛ Testing commit d49d52d with merge fb27e84... |
💔 Test failed - status-travis |
"used by editors" is the purvew of RLS, not really cargo or cargo check.
…On Thu, Dec 15, 2016 at 3:19 PM, Jon Gjengset ***@***.***> wrote:
If the intent is for it to be used by editors to highlight errors inline
(like syntastic or neomake), then it'd be unfortunate if errors inside test
code weren't also highlighted.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3296 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABsivpLiC0BfLzUaiPUuua15OQ67zepks5rIaDDgaJpZM4K0heU>
.
|
What is the intended use-case for |
Right now, I do my work in loops:
Because step 2 is only used for type checking / borrow checking / etc, there's no reason for a final artifact. Now, I can use "cargo check", which will do the same thing, but will be much faster. If you have some sort of IDE-based workflow rather than a terminal based one, then you'd be using your IDEs checking capabilities, which RLS would work with, to do the same thing us non-IDE people do with the first loop. |
I have a workflow that's pretty similar to yours, except that I'd like cargo check to also check that the tests compile. Maybe it'd be sensible for it to only check that that is the case after it after the main code compiles, so that it I don't get extra output from the second compilation, but nonetheless it seems like |
I tried |
I absolutely agree, and I think it's a mistake to think otherwise. @steveklabnik I don't see why using an IDE or not makes any difference in workflow. I use an IDE and the workflow is the same as yours - the only difference is that the IDE invokes cargo for me, and processes the message output and displays the errors in the GUI, not terminal. But it's using cargo underneath just the same. The difference is that my step 2 is If your point was that using an IDE will make using cargo directly obsolete, in favor of using RLS/LSP instead, well... maybe. But it is still far off in the future (at least as the error reporting feature), so we should have a proper solution in the interim, especially if it relatively to have one. The tests should always compile. |
Hi @bruno-medeiros, |
Yes. And if you have 1 million tests in on single crate, the problem you have is not whether to compiler tests or not, but rather, your crate is likely too large. You should split it in smaller ones, decrease the coupling.
An additional note. I think it might be hard to see why there is value in this if one is mainly used to dynamically-typed languages - because those languages have few compile time checks, and in fact delegate a lot of correctness testing to the tests execution themselves. (that is, if the language even has a compile phase at all!) |
You didn't thought about it. |
Just to be clear, I'm not arguing about what the default behaviour of |
@bruno-medeiros, Oh. I misunderstood you. I also want to have an option to check test code. |
This is true in the basic case, but good IDE integration has different requirements.
I phrased this as "basic" vs "excellent", but I don't mean it as a value judgement; some people would prefer the "basic" strategy. I mean it more of an "easy to do" vs "hard to do" scenario, it's "richer" in some sense. "cargo check" is for this "basic" stuff, and RLS is for the "excellent" stuff. The correct tool for the correct use-case. But really, in the end, if you don't see the value of "cargo check", nobody is making you use it 😄 . It has been one of the most-requested cargo commands, so a lot of people do. I don't use any of cargo's vendoring support personally, but that it exists for other people to use is a 👍 , not a 👎 . |
@KalitaAlexey there is a known issue which is that |
re whether tests should be checked. There are clear advantages to both approaches. In practical terms, Now, speaking more philosophically, checking tests too would require sending cfg options to rustc that are not specified by the user. I don't think cargo should do that - it doesn't feel right for a build tool ( |
@steveklabnik I think we are misunderstanding each other. 😅
PS:
Yeah, I know, that's what I meant when I said "displays the errors in the GUI, not terminal". I might have not been clear, but what I meant was: displaying the errors highlighted inline in the editor, hover to get error message, etc., like you said. (I wrote support for that in RustDT, the Rust Eclipse extension 😉 ) I think we all agree Cargo is still going to be used by IDEs and other tools for a while, since |
Hmm, unrelatedly, I'm also seeing a number of erroneous warnings from |
Sounds reasonable. I'd love to see |
Yeah, makes sense to do this as separate item/task. Opened: #3431 |
As a manual test, I tried using the latest rust-lang/cargo to run a
|
…2-14) (from servo:cargoup); r=jdm <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> This version includes the new `cargo check`: rust-lang/cargo#3296 Source-Repo: https://github.com/servo/servo Source-Revision: be896712ac56f69d95b31e0d5a459fbd86f71bd8
Version 1.16.0 (2017-03-16) =========================== Language -------- * Lifetimes in statics and consts default to `'static`. [RFC 1623] * [The compiler's `dead_code` lint now accounts for type aliases][38051]. * [Uninhabitable enums (those without any variants) no longer permit wildcard match patterns][38069] * [Clean up semantics of `self` in an import list][38313] * [`Self` may appear in `impl` headers][38920] * [`Self` may appear in struct expressions][39282] Compiler -------- * [`rustc` now supports `--emit=metadata`, which causes rustc to emit a `.rmeta` file containing only crate metadata][38571]. This can be used by tools like the Rust Language Service to perform metadata-only builds. * [Levenshtein based typo suggestions now work in most places, while previously they worked only for fields and sometimes for local variables][38927]. Together with the overhaul of "no resolution"/"unexpected resolution" errors (#[38154]) they result in large and systematic improvement in resolution diagnostics. * [Fix `transmute::<T, U>` where `T` requires a bigger alignment than `U`][38670] * [rustc: use -Xlinker when specifying an rpath with ',' in it][38798] * [`rustc` no longer attempts to provide "consider using an explicit lifetime" suggestions][37057]. They were inaccurate. Stabilized APIs --------------- * [`VecDeque::truncate`] * [`VecDeque::resize`] * [`String::insert_str`] * [`Duration::checked_add`] * [`Duration::checked_sub`] * [`Duration::checked_div`] * [`Duration::checked_mul`] * [`str::replacen`] * [`str::repeat`] * [`SocketAddr::is_ipv4`] * [`SocketAddr::is_ipv6`] * [`IpAddr::is_ipv4`] * [`IpAddr::is_ipv6`] * [`Vec::dedup_by`] * [`Vec::dedup_by_key`] * [`Result::unwrap_or_default`] * [`<*const T>::wrapping_offset`] * [`<*mut T>::wrapping_offset`] * `CommandExt::creation_flags` * [`File::set_permissions`] * [`String::split_off`] Libraries --------- * [`[T]::binary_search` and `[T]::binary_search_by_key` now take their argument by `Borrow` parameter][37761] * [All public types in std implement `Debug`][38006] * [`IpAddr` implements `From<Ipv4Addr>` and `From<Ipv6Addr>`][38327] * [`Ipv6Addr` implements `From<[u16; 8]>`][38131] * [Ctrl-Z returns from `Stdin.read()` when reading from the console on Windows][38274] * [std: Fix partial writes in `LineWriter`][38062] * [std: Clamp max read/write sizes on Unix][38062] * [Use more specific panic message for `&str` slicing errors][38066] * [`TcpListener::set_only_v6` is deprecated][38304]. This functionality cannot be achieved in std currently. * [`writeln!`, like `println!`, now accepts a form with no string or formatting arguments, to just print a newline][38469] * [Implement `iter::Sum` and `iter::Product` for `Result`][38580] * [Reduce the size of static data in `std_unicode::tables`][38781] * [`char::EscapeDebug`, `EscapeDefault`, `EscapeUnicode`, `CaseMappingIter`, `ToLowercase`, `ToUppercase`, implement `Display`][38909] * [`Duration` implements `Sum`][38712] * [`String` implements `ToSocketAddrs`][39048] Cargo ----- * [The `cargo check` command does a type check of a project without building it][cargo/3296] * [crates.io will display CI badges from Travis and AppVeyor, if specified in Cargo.toml][cargo/3546] * [crates.io will display categories listed in Cargo.toml][cargo/3301] * [Compilation profiles accept integer values for `debug`, in addition to `true` and `false`. These are passed to `rustc` as the value to `-C debuginfo`][cargo/3534] * [Implement `cargo --version --verbose`][cargo/3604] * [All builds now output 'dep-info' build dependencies compatible with make and ninja][cargo/3557] * [Build all workspace members with `build --all`][cargo/3511] * [Document all workspace members with `doc --all`][cargo/3515] * [Path deps outside workspace are not members][cargo/3443] Misc ---- * [`rustdoc` has a `--sysroot` argument that, like `rustc`, specifies the path to the Rust implementation][38589] * [The `armv7-linux-androideabi` target no longer enables NEON extensions, per Google's ABI guide][38413] * [The stock standard library can be compiled for Redox OS][38401] * [Rust has initial SPARC support][38726]. Tier 3. No builds available. * [Rust has experimental support for Nvidia PTX][38559]. Tier 3. No builds available. * [Fix backtraces on i686-pc-windows-gnu by disabling FPO][39379] Compatibility Notes ------------------- * [Uninhabitable enums (those without any variants) no longer permit wildcard match patterns][38069] * In this release, references to uninhabited types can not be pattern-matched. This was accidentally allowed in 1.15. * [The compiler's `dead_code` lint now accounts for type aliases][38051]. * [Ctrl-Z returns from `Stdin.read()` when reading from the console on Windows][38274] * [Clean up semantics of `self` in an import list][38313] [37057]: rust-lang/rust#37057 [37761]: rust-lang/rust#37761 [38006]: rust-lang/rust#38006 [38051]: rust-lang/rust#38051 [38062]: rust-lang/rust#38062 [38062]: rust-lang/rust#38622 [38066]: rust-lang/rust#38066 [38069]: rust-lang/rust#38069 [38131]: rust-lang/rust#38131 [38154]: rust-lang/rust#38154 [38274]: rust-lang/rust#38274 [38304]: rust-lang/rust#38304 [38313]: rust-lang/rust#38313 [38314]: rust-lang/rust#38314 [38327]: rust-lang/rust#38327 [38401]: rust-lang/rust#38401 [38413]: rust-lang/rust#38413 [38469]: rust-lang/rust#38469 [38559]: rust-lang/rust#38559 [38571]: rust-lang/rust#38571 [38580]: rust-lang/rust#38580 [38589]: rust-lang/rust#38589 [38670]: rust-lang/rust#38670 [38712]: rust-lang/rust#38712 [38726]: rust-lang/rust#38726 [38781]: rust-lang/rust#38781 [38798]: rust-lang/rust#38798 [38909]: rust-lang/rust#38909 [38920]: rust-lang/rust#38920 [38927]: rust-lang/rust#38927 [39048]: rust-lang/rust#39048 [39282]: rust-lang/rust#39282 [39379]: rust-lang/rust#39379 [`<*const T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset [`<*mut T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset [`Duration::checked_add`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_add [`Duration::checked_div`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_div [`Duration::checked_mul`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_mul [`Duration::checked_sub`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_sub [`File::set_permissions`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.set_permissions [`IpAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv4 [`IpAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv6 [`Result::unwrap_or_default`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default [`SocketAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv4 [`SocketAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv6 [`String::insert_str`]: https://doc.rust-lang.org/std/string/struct.String.html#method.insert_str [`String::split_off`]: https://doc.rust-lang.org/std/string/struct.String.html#method.split_off [`Vec::dedup_by_key`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by_key [`Vec::dedup_by`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by [`VecDeque::resize`]: https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.resize [`VecDeque::truncate`]: https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.truncate [`str::repeat`]: https://doc.rust-lang.org/std/primitive.str.html#method.repeat [`str::replacen`]: https://doc.rust-lang.org/std/primitive.str.html#method.replacen [cargo/3296]: rust-lang/cargo#3296 [cargo/3301]: rust-lang/cargo#3301 [cargo/3443]: rust-lang/cargo#3443 [cargo/3511]: rust-lang/cargo#3511 [cargo/3515]: rust-lang/cargo#3515 [cargo/3534]: rust-lang/cargo#3534 [cargo/3546]: rust-lang/cargo#3546 [cargo/3557]: rust-lang/cargo#3557 [cargo/3604]: rust-lang/cargo#3604 [RFC 1623]: https://github.com/rust-lang/rfcs/blob/master/text/1623-static.md
Remove unnecessary test This removes a test that is blocking rust-lang/rust#116016 from landing. `@dtolnay` suggested removing the test rather than allowing the relevant lints ([comment](rust-lang/rust#116016 (comment))). > The failure is in https://github.com/rust-lang/cargo/blob/77506e57392d94450bb3ed52cf75e3263bbb2792/tests/testsuite/check.rs#L222-L285 which is a 7 year old test for [#3419](#3419), which is from 2 days after the initial implementation of `cargo check` landed in nightly Cargo in [#3296](#3296). > > I would recommend just deleting the test from Cargo. The implementation of `cargo check` has matured enough since then and I don't see anything useful in that test.
This is not finished - the big omission is no tests, and it needs some more testing too. It also requires rust-lang/rust#37681.However, this is my first non-trivial Cargo patch, so I'd like to get some early feedback.
r? @alexcrichton
and cc @rust-lang/tools since this adds a new Cargo sub-command (although we have previously discussed and approved the idea in a tools meeting).