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

Implement cargo check #2834

Closed
wants to merge 1 commit into from
Closed

Conversation

azerupi
Copy link
Contributor

@azerupi azerupi commented Jul 7, 2016

Attempt to implement #1313

It's the first time I dive into the code of Cargo, so take your time to review this properly. It's mostly copy paste from other commands with little tweaks until it worked like intended (at least I think it does).

Also sorry for the noise, I have rustfmt set up to run on save.

To summarize what I did and why

For linters that are integrated in text editors / IDEs compiling with -Z no-trans reduces the error reporting time. But the problem is that you can't pass custom arguments to rustc when you have multiple targets, for example a lib and a bin or multiple bins. To allow custom arguments to rustc for multiple targets would not be "safe" because cargo has no idea what is passed to rustc and some needed artifacts may or may not be created. See #2642

To allow -Z no-trans on multiple targets without having to remove the restrictions on cargo rustcI have added a new command cargo check

I created a new flag in

pub struct CompileOptions<'a> {
    // [...]
    /// The compilation will stop before the generation of any artifacts: -Z no-trans
    pub compile_check: bool,
}

It is set to false for all commands except cargo check.

In compile_ws I added an extra if for when compile_check is true (this occurs only whit cargo check). When it is true cargo passes -Z no-trans to rustc.

That's it.

@rust-highfive
Copy link

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
Copy link
Member

Thanks for the PR @azerupi! Exciting to see progress on this! Some thoughts of mine:

  • Unfortunately -Z flags will soon not be accessible on the stable channel, so this means that cargo check will only work on nightly. I think that to add a new subcommand on Cargo we need it to work on stable as well, which will likely require work in the compiler to stabilize this flag in some form.
  • I think the semantics as implemented is that if you have a bunch of targets in a crate that cargo check will only check one of them (maybe the library, maybe not?). I think we'll want cargo check to verify all targets in a crate.
  • Could you back out the rustfmt changes? It unfortunately makes it a little difficult to review.

@azerupi
Copy link
Contributor Author

azerupi commented Jul 11, 2016

Hi, thanks for the feedback! :)

Unfortunately -Z flags will soon not be accessible on the stable channel, so this means that cargo check will only work on nightly. I think that to add a new subcommand on Cargo we need it to work on stable as well, which will likely require work in the compiler to stabilize this flag in some form.

Indeed, that would not be useful. If there is some mentoring involved I would be willing to take the jump and try to implement this both on the rustc and cargo side. But it's the first time I work on either code bases, so I will definitely need some guidance and time.

I think the semantics as implemented is that if you have a bunch of targets in a crate that cargo check will only check one of them

This is very much possible, I am not familiar with the code and didn't test it thoroughly enough to say.

Could you back out the rustfmt changes? It unfortunately makes it a little difficult to review.

Of course! However, according to your first point it may be better to set this PR on hold or close it until the necessary changes are made in rustc?

@alexcrichton
Copy link
Member

Hm I was trying to find a tracking issue pertaining to -Z no-trans specifically, but it looks like we don't quite have one just yet. In lieu of that, cc @rust-lang/compiler, any of you aware of progress on the -Z no-trans front in terms of what the next step is for stabilization? Do we want to just expose it as a -C flag or are there more grand plans for perhaps metadata-driven compilation?

@bors
Copy link
Contributor

bors commented Jul 13, 2016

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

@alexcrichton
Copy link
Member

I'm gonna close this for now as we'll need to get the -Z flag onto stable before merging, but I think rust-lang/rust#31847 is the issue to watch for that!

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