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

Remove bundled cargo #157

Merged
merged 4 commits into from
Jul 6, 2022
Merged

Remove bundled cargo #157

merged 4 commits into from
Jul 6, 2022

Conversation

fredszaq
Copy link
Collaborator

@fredszaq fredszaq commented Jun 23, 2022

This commit removes the bundled cargo from dinghy, and instead defers to
the cargo used to launch dinghy in order to actually perform the build.

Dinghy will still setup the environment correctly in order to perform
the cross compilation, but the build is done using the actual cargo and
not one bundled inside dinghy that could be a different version (this
has been known to cause problems in the past). Note that dinghy
registers itself as a runner during the environment setup, which mean
cargo will call dinghy again when it need to run binaries on the target
device.

The benefits to this are huge:

  • we don't have to maintain a copy of the cargo command line inside
    dinghy anymore, which means any flags added inside cargo can be used
    directly when a new version is released without having to update
    dinghy as dinghy will pass the args to cargo as is. This also mean
    any cargo subcommand can be used with dinghy, not just the ones we
    define in dinghy
  • this fixes the discrepancies we had with cargo regarding some of the
    command line flags as we are actually using the real cargo command
    and not reimplementing it ourselves (an example of that is building a
    sub-crate from the root of the workspace while activating features,
    this was utterly broken in dinghy and now works as expected
  • No need to release a version of dinghy each time cargo is updated,
    and having to deal with the internal API changes
  • Compilation is waaaay faster as we don't have to compile cargo
    anymore
  • This removes quite a bit of code

There are however quite a few changes in order to make all of this work:

  • the dinghy specific should now be passed before the subcommand (this
    is the case for --strip, --cleanup, --env, --overlay that
    were previously passed after the subcommand
  • the --bearded argument has been removed (use RUSTC_WRAPPER directly
    if you ever needed this)
  • removed support for [metadata.dinghy] in Cargo.toml, this means
    allowed_rustc_triples and ignored_rustc_triples are not supported
    anymore (use cargo dinghy build --all --ignore if you used
    that)
  • removed support for host device, as this is effectively equivalent to
    using cargo without dinghy but complicated a few things (if you really
    want use dinghy to run code on the host, use an ssh device to localhost)
  • support for running executables that depend on shared libraries build
    within the same workspace has not been implemented, this is doable
    however but seems like a really niche use case so I decided not to
    waste my time on this for now.
  • I didn't test iOS as I don't have a mac

@fredszaq fredszaq force-pushed the remove-bundled-cargo branch 9 times, most recently from a5b07f3 to fe07b2f Compare June 24, 2022 13:49
This commit removes the bundled cargo from dinghy, and instead defers to
the cargo used to launch dinghy in order to actually perfom the build.

Dinghy will still setup the environment correctly in order to perform
the cross compilation, but the build is done using the actual cargo and
not one bundled inside dinghy that could be a different version (this
has been known to cause problems in the past). Note that dinghy
registers itself as a runner during the envinoment setup, which mean
cargo will call dinghy again when it need to run binaries on the target
device.

The benefits to this are huge:
 - we don't have to maintain a copy of the cargo command line inside
   dinghy anymore, which means any flags added inside cargo can be used
   directly when a new version is released without having to update
   dinghy as dinghy will pass the args to cargo as is. This also mean
   any cargo subcommand can be used with dinghy, not just the ones we
   define in dinghy
 - this fixes the discreptancies we had with cargo regarding some of the
   command line flags as we are actually using the real cargo command
   and not reimplementing it ourselves (an example of that is building a
   sub-crate from the root of the workspace while activating features,
   this was utterly broken in dinghy and now works as expected
 - No need to release a version of dinghy each time cargo is updated,
   and having to deal with the internal API changes
 - Compilation is waaaay faster as we don't have to compile cargo
   anymore
 - This removes quite a bit of code

There are however quite a few changes in order to make all of this work:
 - the dinghy specific should now be passed before the subcommand (this
   is the case for `--strip`, `--cleanup`, `--env`, `--overlay` that
   were previously passed after the subcommand
 - the `--bearded` argument has been removed (use RUSTC_WRAPPER directly
   if you ever needed this)
 - removed support for `[metadata.dinghy]` in `Cargo.toml`, this means
   `allowed_rustc_triples` and `ignored_rustc_triples` are not supported
   anymore (use cargo dinghy build --all --ignore <package> if you used
   that)
 - removed support for host device, as this is effectlively equivalant to
   using cargo without dinghy but complicated a few things (if you really
   want use dinghy to run code on the host, use an ssh device to localhost)
 - support for running executables that depend on shared libraries build
   within the same workspace has not been implemented, this is doable
   however but seems like a really nice use case so I decided not to
   waste my time on this for now.
 - I didn't test iOS as I don't have a mac
@fredszaq fredszaq force-pushed the remove-bundled-cargo branch 15 times, most recently from 6ba429a to 043563b Compare June 27, 2022 12:05
@fredszaq fredszaq force-pushed the remove-bundled-cargo branch from 20983c4 to a33b400 Compare June 27, 2022 12:45
@fredszaq
Copy link
Collaborator Author

this fixes #39

@fredszaq
Copy link
Collaborator Author

Hey @madsmtm @simlay @EliseChouleur @Robert-Steiner ! You all opened PRs on dinghy in the last year so I guess you are using it ! As this PR includes quite a few big changes, it would be awesome if you could test it and confirm it does not completely break your use cases (it shouldn't but we can't be too cautious)

@simlay
Copy link
Contributor

simlay commented Jul 1, 2022

I tested the iOS simulator support for this PR with some of the stuff in uikit-sys. I didn't test on device - I'm not actually sure this works currently outside of this PR though.

@kali
Copy link
Collaborator

kali commented Jul 1, 2022

@simlay no, ios-on-device does not work on main either. I'm slowly gathering motivation to have a look.

@fredszaq
Copy link
Collaborator Author

fredszaq commented Jul 1, 2022

Ok so apart for the broken iOS device support that is also broken on main, nothing broken on your side @simlay ? Thanks a lot for taking the time to test !

@simlay
Copy link
Contributor

simlay commented Jul 1, 2022

Ok so apart for the broken iOS device support that is also broken on main, nothing broken on your side @simlay ? Thanks a lot for taking the time to test !

Sure thing! Yeah, I think there's nothing broken for me. I tested it with the aarch64 iOS simulator and the x86_64 iOS simulator.

Also, love the addition of android (even if I don't use it) to CI.

@kali kali merged commit 31aafd4 into main Jul 6, 2022
@kali kali deleted the remove-bundled-cargo branch July 6, 2022 09:41
@madsmtm
Copy link
Contributor

madsmtm commented Jul 30, 2022

Sorry I didn't answer earlier, my setup's been broken for a while but got things set up again now, the updated version works perfectly for me, nice work!

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