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

Add cross subcommands shortcuts #445

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

sigmaSd
Copy link
Contributor

@sigmaSd sigmaSd commented Jul 16, 2020

Hi, this pr adds sub-commands shortcuts ['b','c','r', 't'] to the cli.
The main reason is currently the error when using them is misleading.
Example:
this command:
cross b --target arm-linux-androideabi
outputs this error:
error: linking with cc failed: exit code: 1
which took me a while to figure out that cross is not recognizing the sub-command and is falling-back to cargo.
Also they are useful to have.

@sigmaSd sigmaSd requested review from Dylan-DPC-zz and a team as code owners July 16, 2020 16:14
@sigmaSd sigmaSd changed the title Add cargo subcommands shortcuts Add cross subcommands shortcuts Jul 16, 2020
@Emilgardis
Copy link
Member

Thank you for your contribution!

This seems like it should be the default behaviour, but should cross maybe support all aliases, including userdefined aliases?

In cargo, the b,c,r,t aliases are shadowed by user-defined aliases.

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Jul 17, 2020

I agree it makes sense. The issue here is to handle aliases 100% there are a lot of things to account for https://doc.rust-lang.org/cargo/reference/config.html,
1- handling config file location
2- handling aliases from environment variable
3- parsing the config file
And all of these have a lot of edge cases. As far as I see so far cross tried to avoid this complexity and delegate most of this to cargo
The good thing is -from the docs- Aliases are not allowed to redefine existing built-in commands. which mean that misleading errors when building or running should not happen

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Aug 13, 2020
445: Add cross subcommands shortcuts r=Emilgardis a=sigmaSd

Hi, this pr adds sub-commands shortcuts `['b','c','r', 't']` to the cli.
The main reason is currently the error when using them is misleading.
Example:
this command:
`cross b --target arm-linux-androideabi`
outputs this error: 
`error: linking with `cc` failed: exit code: 1` 
which took me a while to figure out that cross is not recognizing the sub-command and is falling-back to cargo.
Also they are useful to have.


Co-authored-by: Nbiba Bedis <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 13, 2020

Build failed:

@Emilgardis
Copy link
Member

bors retry

bors bot added a commit that referenced this pull request Sep 18, 2020
445: Add cross subcommands shortcuts r=Emilgardis a=sigmaSd

Hi, this pr adds sub-commands shortcuts `['b','c','r', 't']` to the cli.
The main reason is currently the error when using them is misleading.
Example:
this command:
`cross b --target arm-linux-androideabi`
outputs this error: 
`error: linking with `cc` failed: exit code: 1` 
which took me a while to figure out that cross is not recognizing the sub-command and is falling-back to cargo.
Also they are useful to have.


Co-authored-by: Nbiba Bedis <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 19, 2020

Build failed:

  • rust-embedded.cross

@domenicquirl
Copy link

Hi! I just spent a good while tearing my hair out over this, because I was trying to run cross b on a system where the corresponding --target was also installed and the command cause a regular build to run and complete without error (context: cross-compiling from a linux system to armv7, having installed the toolchain and a matching linker on the source machine to test the build locally). Got it figured out, but landed here in the process and I think this landing would help avoid such situations for others the future.

@reitermarkus
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 14, 2022

Build succeeded:

@bors bors bot merged commit aaad50c into cross-rs:main Mar 14, 2022
@Emilgardis Emilgardis added this to the v0.2.2 milestone Jun 15, 2022
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