-
Notifications
You must be signed in to change notification settings - Fork 423
Simplify Target to use only to cases: BuiltIn and Custom #217
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
Conversation
In the current form, cross needs to known every target its supports. This makes things too restrict. For example, its not possible to use a custom target image for a target that cross does not provides a docker image (like aarch64-unknown-linux-musl, for example). Cross refuses to execute saying it does not support the specified target. Besides that, to add a new docker image the cross code needs to be changed. This changes simplifies the Target enum to have only two variants: BuiltIn and Custom. Cross needs to known if a target is a custom target so it can call xargo, otherwise, all BuitIn targets can be uniformly handled.
Appreciate a second review from @jamesmunns or @rust-embedded/Tools |
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.
provided the tests all pass, lgtm!
bors: r+ |
217: Simplify Target to use only to cases: BuiltIn and Custom r=Dylan-DPC a=malbarbo In the current form, cross needs to known every target its supports. This makes things too restrict. For example, its not possible to use a custom target image for a target that cross does not provides a docker image (like aarch64-unknown-linux-musl, for example). Cross refuses to execute saying it does not support the specified target. Besides that, to add a new docker image the cross code needs to be changed. This changes simplifies the Target enum to have only two variants: BuiltIn and Custom. Cross needs to known if a target is a custom target so it can call xargo, otherwise, all BuitIn targets can be uniformly handled. Co-authored-by: Marco A L Barbosa <malbarbo@gmail.com>
Build failed |
@malbarbo Fails on one target
|
The target x86_64-unknown-dragonfly is not available on rustup, so it also needs xargo. Can we try again? |
Sure. though i feel that having |
bors: r+ |
217: Simplify Target to use only to cases: BuiltIn and Custom r=Dylan-DPC a=malbarbo In the current form, cross needs to known every target its supports. This makes things too restrict. For example, its not possible to use a custom target image for a target that cross does not provides a docker image (like aarch64-unknown-linux-musl, for example). Cross refuses to execute saying it does not support the specified target. Besides that, to add a new docker image the cross code needs to be changed. This changes simplifies the Target enum to have only two variants: BuiltIn and Custom. Cross needs to known if a target is a custom target so it can call xargo, otherwise, all BuitIn targets can be uniformly handled. Co-authored-by: Marco A L Barbosa <malbarbo@gmail.com>
Build failed |
Your latest change broke every platform 😄 |
This adjust AvaliableTargets to separate installed and not installed targets so we do not attempt to install a installed target.
db3f4ec
to
6fa1542
Compare
I've adjusted AvaliableTargets (see the commit message) and remove the needs_xargo flag. |
bors: r+ |
217: Simplify Target to use only to cases: BuiltIn and Custom r=Dylan-DPC a=malbarbo In the current form, cross needs to known every target its supports. This makes things too restrict. For example, its not possible to use a custom target image for a target that cross does not provides a docker image (like aarch64-unknown-linux-musl, for example). Cross refuses to execute saying it does not support the specified target. Besides that, to add a new docker image the cross code needs to be changed. This changes simplifies the Target enum to have only two variants: BuiltIn and Custom. Cross needs to known if a target is a custom target so it can call xargo, otherwise, all BuitIn targets can be uniformly handled. Co-authored-by: Marco A L Barbosa <malbarbo@gmail.com>
Build failed |
Dragonfly strikes again. Same issue as we had previously |
bors: try |
The same issue, but a different reason. I tested dragonfly locally. Let's see how is goes. |
tryBuild succeeded |
bors: r+ |
👎 Rejected by too few approved reviews |
bors: r+ |
217: Simplify Target to use only to cases: BuiltIn and Custom r=Dylan-DPC a=malbarbo In the current form, cross needs to known every target its supports. This makes things too restrict. For example, its not possible to use a custom target image for a target that cross does not provides a docker image (like aarch64-unknown-linux-musl, for example). Cross refuses to execute saying it does not support the specified target. Besides that, to add a new docker image the cross code needs to be changed. This changes simplifies the Target enum to have only two variants: BuiltIn and Custom. Cross needs to known if a target is a custom target so it can call xargo, otherwise, all BuitIn targets can be uniformly handled. Co-authored-by: Marco A L Barbosa <malbarbo@gmail.com>
Build succeeded |
431: Allow users to pick cargo for custom target builds r=reitermarkus a=anupdhml This is an attempt to fix #325. We want to use the target `x86_64-alpine-linux-musl` (available from alpine's rustc) to build musl-based binaries and when using cross with something not in the standard rustc/rustup targets, cross errors out as detailed in the issue above. For a non-standard target, cross defaults to using `xargo` right now (behavior introduced in #217), but in our case, since we want `cargo` to run ultimately, I had to also ensure that setting `xargo = false` actually works from the cross config file. If there's a better way of handling this without breaking the current default behavior, please let me know. Also updated the docs on xargo usage accordingly (including the parts that were lagging even without this PR). --- A working example of cross use based on the changes here are available at: https://github.com/wayfair-tremor/tremor-runtime/blob/f72e133f971adca67f6e6b6e265a4f5d5e96380d/Cross.toml#L30-L37 --- Thanks for providing a tool like cross btw! It's been really useful to us as we are looking to streamline our release process 😃 Co-authored-by: Anup Dhamala <anupdhml+git@gmail.com>
In the current form, cross needs to known every target its supports.
This makes things too restrict. For example, its not possible to use a
custom target image for a target that cross does not provides a docker
image (like aarch64-unknown-linux-musl, for example). Cross refuses to
execute saying it does not support the specified target. Besides that,
to add a new docker image the cross code needs to be changed.
This changes simplifies the Target enum to have only two variants:
BuiltIn and Custom. Cross needs to known if a target is a custom target
so it can call xargo, otherwise, all BuitIn targets can be uniformly
handled.