Skip to content

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

Merged
merged 3 commits into from
Oct 11, 2018

Conversation

malbarbo
Copy link
Contributor

@malbarbo malbarbo commented Oct 9, 2018

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.

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.
Dylan-DPC-zz
Dylan-DPC-zz previously approved these changes Oct 9, 2018
@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Oct 9, 2018

Appreciate a second review from @jamesmunns or @rust-embedded/Tools

ryankurte
ryankurte previously approved these changes Oct 10, 2018
Copy link

@ryankurte ryankurte left a 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!

@Dylan-DPC-zz
Copy link

bors: r+

bors bot added a commit that referenced this pull request Oct 10, 2018
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>
@bors
Copy link
Contributor

bors bot commented Oct 10, 2018

Build failed

@Dylan-DPC-zz
Copy link

@malbarbo Fails on one target

error[E0463]: can't find crate for `core`
  |
  = note: the `x86_64-unknown-dragonfly` target may not be installed

@malbarbo malbarbo dismissed stale reviews from ryankurte and Dylan-DPC-zz via db3f4ec October 10, 2018 13:58
Dylan-DPC-zz
Dylan-DPC-zz previously approved these changes Oct 10, 2018
@malbarbo
Copy link
Contributor Author

The target x86_64-unknown-dragonfly is not available on rustup, so it also needs xargo. Can we try again?

@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Oct 10, 2018

Sure. though i feel that having uses_xargo and needs_xargo variables feels a odd and confusing to me. We can refactor it later though.

@Dylan-DPC-zz
Copy link

bors: r+

bors bot added a commit that referenced this pull request Oct 10, 2018
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>
@bors
Copy link
Contributor

bors bot commented Oct 10, 2018

Build failed

@Dylan-DPC-zz
Copy link

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.
@malbarbo
Copy link
Contributor Author

I've adjusted AvaliableTargets (see the commit message) and remove the needs_xargo flag.

Dylan-DPC-zz
Dylan-DPC-zz previously approved these changes Oct 11, 2018
@Dylan-DPC-zz
Copy link

bors: r+

bors bot added a commit that referenced this pull request Oct 11, 2018
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>
@bors
Copy link
Contributor

bors bot commented Oct 11, 2018

Build failed

@Dylan-DPC-zz
Copy link

Dragonfly strikes again. Same issue as we had previously

@Dylan-DPC-zz
Copy link

bors: try

bors bot added a commit that referenced this pull request Oct 11, 2018
@malbarbo
Copy link
Contributor Author

The same issue, but a different reason. I tested dragonfly locally. Let's see how is goes.

@bors
Copy link
Contributor

bors bot commented Oct 11, 2018

try

Build succeeded

@Dylan-DPC-zz
Copy link

bors: r+

@bors
Copy link
Contributor

bors bot commented Oct 11, 2018

👎 Rejected by too few approved reviews

@Dylan-DPC-zz
Copy link

bors: r+

bors bot added a commit that referenced this pull request Oct 11, 2018
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>
@bors
Copy link
Contributor

bors bot commented Oct 11, 2018

Build succeeded

@bors bors bot merged commit 29bc75d into cross-rs:master Oct 11, 2018
@malbarbo malbarbo deleted the simplify-target branch October 16, 2018 13:54
bors bot added a commit that referenced this pull request Jun 12, 2020
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>
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.

3 participants