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

rust-cargo checks tests, examples and benches targets #1206

Merged
merged 2 commits into from
Mar 22, 2017

Conversation

fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Feb 7, 2017

Previously, rust-cargo was unable to check integration tests and other
optional Cargo targets. For a description of the conventional Cargo layout,
see:

http://doc.crates.io/manifest.html#the-project-layout

This commits enhances rust-cargo to check all files under the conventional
layout, by using flycheck-rust-crate-type as the target type, and
flycheck-rust-binary-name as the target name.

In addition, it adds :enabled and :verify properties to rust-cargo to ensure
these variables are correctly set, and to inform the user in case they are not.

Integration tests are added to check the support for the full conventional
layout.

@manuel-uberti
Copy link
Contributor

I let others more qualified do the review, but you're making an impressive work for Rust support in Flycheck. Great stuff.

@fmdkdd
Copy link
Member Author

fmdkdd commented Feb 7, 2017

@manuel-uberti Well, thank you! I'm glad to help.

@fmdkdd fmdkdd force-pushed the rust-cargo-full-layout branch 2 times, most recently from dd634bd to d68397e Compare February 9, 2017 11:43
fmdkdd added a commit to flycheck/flycheck-rust that referenced this pull request Feb 9, 2017
As flycheck/flycheck#1206 has added support for the full conventional project
layout, flycheck-rust should be able to automatically set the variables for
all files in the project as well.

This commit uses `cargo --read-manifest` to get a list of build targets for the
project, and try to determine the correct values for the -rust-crate-type and
-rust-binary-name variables.

As Cargo doesn't actually read the files to determine the targets, we have no
way to know if a file will be compiled, but if the project follows the
conventional layout, we should get it right most of the time.  If not, users can
always override the variables using local variables.

This commit adds test to make sure the association of files to targets works on
a dummy project.
fmdkdd added a commit to flycheck/flycheck-rust that referenced this pull request Feb 9, 2017
As flycheck/flycheck#1206 has added support for the full conventional project
layout, flycheck-rust should be able to automatically set the variables for
all files in the project as well.

This commit uses `cargo --read-manifest` to get a list of build targets for the
project, and try to determine the correct values for the -rust-crate-type and
-rust-binary-name variables.

As Cargo doesn't actually read the files to determine the targets, we have no
way to know if a file will be compiled, but if the project follows the
conventional layout, we should get it right most of the time.  If not, users can
always override the variables using local variables.

This commit adds test to make sure the association of files to targets works on
a dummy project.
@fmdkdd
Copy link
Member Author

fmdkdd commented Mar 13, 2017

@cpitclaudel Any chance of reviewing this one soon? The tests are lengthy, but they all do the same thing. I really need a pair of eyes on the changes to the checker, especially :verify.

@cpitclaudel
Copy link
Member

I'll do that right away. Sorry for the delay.

values.

For `rust', the value should be a string denoting the crate type
for the `--crate-type' flag of rustc."
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to make this a string rather than an explicit enumeration (with a :choice of symbols)? We could intern old values for compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I didn't know about :choice. But we are using flycheck-rust-crate-type for both the rust and rust-cargo checkers, and they have different allowed values:

For rust:

bin|lib|rlib|dylib|cdylib|staticlib|metadata

For rust-cargo:

lib|bin|example|test|bench

So depending on which checker you use, some values wouldn't make sense. Leaving it as a string and letting the checker verify the value seemed an okay compromise.

flycheck.el Outdated
A valid Cargo target type is one of `lib', `bin', `example',
`test' or `bench'."
(member crate-type '("lib" "bin" "example" "test" "bench")))

Copy link
Member

Choose a reason for hiding this comment

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

(then this wouldn't be needed)

flycheck.el Outdated
:predicate flycheck-buffer-saved-p
:enabled (lambda ()
(and (locate-dominating-file (buffer-file-name) "Cargo.toml")
(flycheck-rust-valid-crate-type-p flycheck-rust-crate-type)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this type check belongs here — we don't do this for other checkers, AFAIK.

Copy link
Member Author

Choose a reason for hiding this comment

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

The check to rust-valid-crate-type? Well, :enabled is rather recent, so not all checkers use it. My thinking was that if we enable the checker with an invalid crate type, the user will get a suspicious checker state. Since we have :verify, we might as well disable a checker with an invalid configuration.

Though there's an unforeseen side-effect: disabling the checker will allow for the plain rust checker to run on the file. So I'm not sure anymore if that's a better situation :/

flycheck.el Outdated
(and (locate-dominating-file (buffer-file-name) "Cargo.toml")
(flycheck-rust-valid-crate-type-p flycheck-rust-crate-type)
(or (string= flycheck-rust-crate-type "lib")
flycheck-rust-binary-name)))
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that flycheck won't run by default unless a flycheck-rust-binary-name is buffer- or dir-locally set?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, and this change is too restrictive as cargo will build the default target if you don't pass any flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addendum: cargo will build the default target even when there are multiple targets available, but cargo rustc (which we use in the checker) will complain when there are multiple targets. Still, I can change it so that we don't pass any flag when the variables are set to nil.

flycheck.el Outdated
flycheck-rust-binary-name)))
:verify (lambda (_)
(let* ((toml (locate-dominating-file
(buffer-file-name) "Cargo.toml"))
Copy link
Member

Choose a reason for hiding this comment

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

I think this will throw errors when buffer-file-name is nil.

flycheck.el Outdated
:verify (lambda (_)
(let* ((toml (locate-dominating-file
(buffer-file-name) "Cargo.toml"))
(crate-type flycheck-rust-crate-type)
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this alias here?

@cpitclaudel
Copy link
Member

This looks good (I left a bunch of comments), but I'm surprised: does this mean the rust checker won't be usable anymore without an explicit flycheck-rust-binary-name?

@fmdkdd
Copy link
Member Author

fmdkdd commented Mar 14, 2017

I've pushed changes to allow the checker to run without a binary name and without a crate type. I've also added a test to catch this situation.

I've made accompanying changes to :enabled and :verify, dropped the superfluous aliases, and checker for (buffer-file-name).

Remaining issues:

  1. Should we use choice for flycheck-rust-crate-type?
  2. Should we disable the checker in the case of an invalid user configuration?

@cpitclaudel
Copy link
Member

Thanks!

Should we use choice for flycheck-rust-crate-type?

I'll defer to you on this. One last question that might orient the choice: if the two tools take different values, should the option make it possible to specify two different values too? As in, should we have an second option that defaults to the value of the first one?

Should we disable the checker in the case of an invalid user configuration?

I'd like to keep quick-running status checks in :verify rather than in :enabled, since they cost little and don't require reverting the buffer to redo the checks. In the long run it would be nice to add a "re-check" button in the status buffer.

@fmdkdd
Copy link
Member Author

fmdkdd commented Mar 17, 2017

should we have an second option that defaults to the value of the first one?

I don't think there's a realistic use case where you would want to have the rust and rust-cargo checkers running on the same file. So, there's no need for setting two values. For the record, I would rather use choice and avoid flycheck-rust-valid-crate-type-p for simplicity, but since we have these two sets of values, I really think leaving it as a string is best here.

I'd like to keep quick-running status checks in :verify rather than in :enabled

My idea in adding these checks to :enabled was to 1) avoid doing unnecessary checks in :predicate and 2) avoid running the checker with a bogus configuration.

If the user sets the flycheck-rust-crate-type and flycheck-rust-binary-name incorrectly, they are greeted with a suspicious checker state message and an advice to update Cargo. This is not really helpful, since the error lies in the user configuration of these Flycheck variables.

But, now that I think of it, we won't catch "correct" configurations that do not correspond to valid targets in the project. So, after all, I've revised my judgement and think it might be best to pass the variables to Cargo and let the user deal with it.

Since flycheck-rust will take care of setting the variables correstly most of the time, this shouldn't be an issue. And disabling the checker is a bit unpractical.

I've cut the :enabled checks to only include the existence of the Cargo.toml file. I've left :verify as is. What do you think?

@cpitclaudel
Copy link
Member

Looks good, thanks. I'm still not entirely sure why :choice wouldn't be as good as string (offering the same list of choices as we currently check against), but I'm happy to trust you on this :)

@fmdkdd
Copy link
Member Author

fmdkdd commented Mar 18, 2017

  1. Say we put the list nil|lib|bin|example|test|bench as :choice for flycheck-rust-crate-type. Is the user able to put rlib in the same variable? Not from Customize I guess, but still from setq? They must be able to set values accepted by the plain rust checker as well. If they can't, this is too restrictive. If they can, then we must still check in :verify if the value is one that works for rust-cargo, so we keep flycheck-rust-crate-type-p.

  2. Say we put the list nil|example|test|bench|bin|lib|rlib|dylib|cdylib|staticlib|metadata instead, merging the two sets of values. We still have to check the subset allowed by rust-cargo with the predicate.

So unless I'm mistaken, having :choice does not mean we can get rid of the predicate. But I guess option 2 is a bit nicer for users of Customize.

@cpitclaudel
Copy link
Member

I think that's correct :) It still requires a predicate, but option 2 is nicer for users of customize (and we can add text labels that indicate for which option which checker(s) it's valid for.

Previously, `rust-cargo` was unable to check integration tests and other
optional Cargo targets.  For a description of the conventional Cargo layout,
see:

http://doc.crates.io/manifest.html#the-project-layout

This commits enhances `rust-cargo` to check all files under the conventional
layout, by using `flycheck-rust-crate-type` as the target type, and
`flycheck-rust-binary-name` as the target name.

In addition, it adds :enabled and :verify properties to `rust-cargo` to ensure
these variables are correctly set, and to inform the user in case they are not.

Integration tests are added to check the support for the full conventional
layout.
@fmdkdd
Copy link
Member Author

fmdkdd commented Mar 19, 2017

Is this what you had in mind?

-  :type 'string
+  :type '(choice (const :tag "nil (rust/rust-cargo)" nil)
+                 (const :tag "lib (rust/rust-cargo)" "lib")
+                 (const :tag "bin (rust/rust-cargo)" "bin")
+                 (const :tag "example (rust-cargo)" "example")
+                 (const :tag "test (rust-cargo)" "test")
+                 (const :tag "bench (rust-cargo)" "bench")
+                 (const :tag "rlib (rust)" "rlib")
+                 (const :tag "dylib (rust)" "dylib")
+                 (const :tag "cdylib (rust)" "cdylib")
+                 (const :tag "staticlib (rust)" "staticlib")
+                 (const :tag "metadata (rust)" "metadata"))

@fmdkdd fmdkdd merged commit 33df257 into master Mar 22, 2017
@fmdkdd fmdkdd deleted the rust-cargo-full-layout branch March 22, 2017 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants