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

Add variable to fix rust-cargo error for multiple targets #958

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@fmdkdd
Member

fmdkdd commented May 7, 2016

When multiple binary targets are present, cargo rustc requires the --bin NAME option to filter the target to build, otherwise it errs.

This variable allows one to specify the binary to build for projects that have multiple targets. If it is unset, we fallback to the previous behavior of not passing --bin, so it shouldn't break anything.

And I've also updated the 'multiline-error' test, which failed because rustc updated its syntax.

fmdkdd added some commits May 7, 2016

Fix the 'multiline-error' rust test
Error reports from rustc have changed again (see GH-592).  I updated the
test to match the new format.
Add `binary-name` variable to rust-cargo
When there are multiple targets, we should call `cargo rustc --bin NAME`
with NAME being the binary name to build.

This new variable allows one to set the binary name.  rust-cargo will
only use it when `flycheck-rust-crate-type` is `bin`.  If it is unset,
we fallback to the previous behavior of not using `--bin`, to let cargo
build the default binary.
flycheck.el Outdated
`bin' and there are multiple binary targets."
:type 'string
:safe #'stringp
:package-version '("flycheck" . "0.28"))

This comment has been minimized.

@lunaryorn

lunaryorn May 7, 2016

Contributor

The version would be 27 here.

If I may ask, why did you use 0.28 here?

This comment has been minimized.

@fmdkdd

fmdkdd May 8, 2016

Member

I misinterpreted 0.27snapshot as being before the 0.28 release.

flycheck.el Outdated
"The name of the binary to pass to `cargo rustc --bin'.
The value of this variable is a string denoting the name of the
binary to build: either the name of the crate, or the name of one

This comment has been minimized.

@lunaryorn

lunaryorn May 7, 2016

Contributor

Capitalize "either" here :)

`rust-cargo` needs this option to correctly invoke cargo when
`flycheck-rust-crate-type` is ``bin`` in the presence of multiple
binary targets.

This comment has been minimized.

@lunaryorn

lunaryorn May 7, 2016

Contributor

Replace this entire paragraph with Only required when flycheck-rust-crate-type is bin. This page only provides an overview over all options, we shouldn't go into too much detail here. Details go into the docstrings 😃

This comment has been minimized.

@fmdkdd

fmdkdd May 8, 2016

Member

Okay for brevity. But the variable is only checked when the crate-type is bin /and/ when there are multiple targets. On single targets, we don't need to pass the binary name (previous behavior).

flycheck.el Outdated
@@ -8127,7 +8140,11 @@ Relative paths are relative to the file being checked."
This syntax checker needs Cargo with rustc subcommand."
:command ("cargo" "rustc"
(eval (if (string= flycheck-rust-crate-type "lib") "--lib" nil))
(eval (if (string= flycheck-rust-crate-type "lib")

This comment has been minimized.

@lunaryorn

lunaryorn May 7, 2016

Contributor

Would you mind to replace these nested diffs with a single cond?

This comment has been minimized.

@fmdkdd

fmdkdd May 8, 2016

Member

With pleasure :)

flycheck.el Outdated
of the files under `src/bin'.
This variable is needed only when `flycheck-rust-crate-type' is
`bin' and there are multiple binary targets."

This comment has been minimized.

@lunaryorn

lunaryorn May 7, 2016

Contributor

Please rephrase this sentence a bit to emphasise that this option has only effect if flycheck-rust-crate-type is bin.

@@ -4124,7 +4124,8 @@ Why not:
:checker ruby-jruby))))
(flycheck-ert-def-checker-test rust-cargo rust warning
(let ((flycheck-rust-crate-type "bin"))
(let ((flycheck-rust-crate-type "bin")
(flycheck-rust-binary-name "flycheck"))

This comment has been minimized.

@lunaryorn

lunaryorn May 7, 2016

Contributor

Do we need this option here?

This comment has been minimized.

@fmdkdd

fmdkdd May 8, 2016

Member

We don't. I initially passed --bin unconditionally, which required a binary name. But then I changed the test in the checker to fallback to the previous behavior of not specifying any binary name, and forgot to remove this line.

Update after code review
- Rewording of the docstring
- Change package-version to 0.27
- Use `cond` instead of nested `if`s
- Remove unnecessary variable in 'warning' test

@lunaryorn lunaryorn self-assigned this May 9, 2016

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 9, 2016

@fmdkdd Ah, I wanted to merge this, but I noticed that your Github profile name is the same as your Github nickname, and I wondered under which name to our contributor's list. Your Github alias, or would you rather like to appear under a different name?

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented May 10, 2016

@lunaryorn My alias is fine, if that's ok with you.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 10, 2016

@fmdkdd Sure.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 10, 2016

@fmdkdd I've merged this change, and will soon merge the corresponding PR to flycheck-rust.

I've sent you an invitation to our Github org which gives you push access to this repo and to flycheck-rust. Please take a look at our Contributor's Guide and our Maintainer's Guide to see how we work and what guidelines to follow when pushing to Flycheck.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented May 10, 2016

@lunaryorn Thank you for the invitation. I will take a look at the guides and do my best to lend a hand.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 10, 2016

@fmdkdd Great, awesome to have you on board! /cc @flycheck/maintainers

@fmdkdd fmdkdd deleted the fmdkdd:rust-cargo-fix branch May 10, 2016

lunaryorn added a commit to flycheck/flycheck-rust that referenced this pull request May 11, 2016

Fix error related to multiple binary targets
When both 'src/lib.rs' and 'src/main.rs' are present in the project, we
default to invoking `cargo rustc`.  This command requires an explicit
binary name with the `--bin` flag, otherwise it causes the error
described in GH-23.

This commit adds logic to parse the output of `cargo read-manifest` to
determine the target type to check, based on the buffer file name.  If
the file name matches one of the targets, this target is chosen.
Otherwise, the first target is chosen as a default.

If the target is a binary, we also extract the binary name and set a new
`flycheck-rust-binary-name` variable (see flycheck/flycheck#958) to
solve the error.

I've tested this work with 'src/main.rs' binaries as well as multiples
binaries under 'src/bin'.

Closes GH-25, fixes GH-23, and adds logic to tackle GH-8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment