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

Update embassy dependencies #19

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

petekubiak
Copy link
Contributor

This PR updates the embassy dependencies to the latest version, so that ector can be used with the latest version of embassy (without these changes there is a dependency clash with embassy v0.6.0).
We have also removed the #[feature] flags which have now been made stable, meaning that the repo as a whole no longer requires the nightly toolchain.
As well as code review, we are looking for input on a couple of points to do with this PR:

  • Should the crate version number be bumped as part of this PR? If so, would it become v0.6.0 or v0.5.1?
  • There is a build issue running the tests with these changes: the linker reports the following:
/usr/bin/ld: /home/pkubiak/repos/private-ector/target/debug/deps/libembassy_time_driver-8a822e1ce30195a2.rlib(embassy_time_driver-8a822e1ce30195a2.embassy_time_driver.656d4db9a7f09f27-cgu.0.rcgu.o): in function `embassy_time_driver::now':
          /home/pkubiak/.cargo/registry/src/index.crates.io-6f17d22bba15001f/embassy-time-driver-0.1.0/src/lib.rs:146: undefined reference to `_embassy_time_now'
          /usr/bin/ld: /home/pkubiak/repos/private-ector/target/debug/deps/libembassy_time_driver-8a822e1ce30195a2.rlib(embassy_time_driver-8a822e1ce30195a2.embassy_time_driver.656d4db9a7f09f27-cgu.0.rcgu.o): in function `embassy_time_driver::allocate_alarm':
          /home/pkubiak/.cargo/registry/src/index.crates.io-6f17d22bba15001f/embassy-time-driver-0.1.0/src/lib.rs:153: undefined reference to `_embassy_time_allocate_alarm'
          /usr/bin/ld: /home/pkubiak/repos/private-ector/target/debug/deps/libembassy_time_driver-8a822e1ce30195a2.rlib(embassy_time_driver-8a822e1ce30195a2.embassy_time_driver.656d4db9a7f09f27-cgu.0.rcgu.o): in function `embassy_time_driver::set_alarm_callback':
          /home/pkubiak/.cargo/registry/src/index.crates.io-6f17d22bba15001f/embassy-time-driver-0.1.0/src/lib.rs:158: undefined reference to `_embassy_time_set_alarm_callback'
          /usr/bin/ld: /home/pkubiak/repos/private-ector/target/debug/deps/libembassy_time_driver-8a822e1ce30195a2.rlib(embassy_time_driver-8a822e1ce30195a2.embassy_time_driver.656d4db9a7f09f27-cgu.0.rcgu.o): in function `embassy_time_driver::set_alarm':
          /home/pkubiak/.cargo/registry/src/index.crates.io-6f17d22bba15001f/embassy-time-driver-0.1.0/src/lib.rs:163: undefined reference to `_embassy_time_set_alarm'
          collect2: error: ld returned 1 exit status

We are not familiar enough with embassy to know where to look for the source of this problem, does anyone have any ideas? The examples can be built and run without a problem, it is only the tests which report this issue.

@lulf
Copy link
Member

lulf commented Oct 18, 2024

That's a good question, I've cloned your PR and tried to fix it, but I'm not sure what could be the problem here. This must be some kind of cargo resolving issue, because cargo tree --format '{p} {f}' shows that the features on embassy-time for enabling the builtin timer is set.

TBH I don't think the integration tests add any value anymore now that the examples are much easier to run than they used to be.

@jamessizeland
Copy link

happy for me to remove the integration test?

@lulf
Copy link
Member

lulf commented Oct 18, 2024

I think it's ok to remove, they don't add much value and has been a source of annoyance for a long time.

@jamessizeland
Copy link

I might see if it can run in tokio instead first

@jamessizeland
Copy link

no idea! I'll remove and update the PR.

@petekubiak
Copy link
Contributor Author

There's a clippy issue which needs resolving too, it's not in a file we've changed but clippy issues are set as errors. I noticed earlier there's another PR from February which was also aiming to update dependencies, and contains what I now realise is the fix for the clippy issue. Here is the relevant fix:
23f79bf#diff-0e3fec3ae933f1e8228e1156fc37bb667d6c41ed64ed562558d009fccc9860f0

@jamessizeland
Copy link

Cool, I'll leave it for you to look at when you're back @petekubiak. We can consolidate the bits from the other PRs too I guess.

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