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

CI and devshell #2916

Closed
wants to merge 2 commits into from
Closed

CI and devshell #2916

wants to merge 2 commits into from

Conversation

multivac61
Copy link

I'm opening this PR for a number of reasons:

  1. Making implicit dependencies explicit, e.g., version of probe-rs, cargo (by using rust-toolchain.toml / rust-toolchain-nightly.toml. Allow every developer of the project to have the exact same toolchain, et cetera.
  2. Simplify the adoption of embassy for newcomers. Everything you need is a nix develop away
  3. CI: basic formatting and linting for the whole project (see nix/formatters.nix for whole list). Later we can also build the whole project, examples and all, using Github Actions or another open platform.
  4. Reproducible builds and caching: While not implemented yet, this PR opens the ability to build embassy project reproducibly manner.

Open to suggestions. Love the project!

@multivac61
Copy link
Author

Why is this a private app?

CleanShot 2024-05-07 at 13 24 58@2x

@lulf
Copy link
Member

lulf commented May 13, 2024

First of all thanks for taking the time to improve this. I'm a bit divided if we should start to add such development environment-specific things like nix configs to the repo. We already have .vscode, but this has a lot of users so I think that's ok.

Isn't there a way to keep these out-repo in a usable form to ensure we don't end up with tons of files like these which we have no way to maintain?

@multivac61
Copy link
Author

Thank you @lulf!

Ultimately there are both pros and cons to having the developer environment as part of the repo: The good is consistent environments for all (if it works on my machine it works on yours), you get single-source of truth formatting and linting, you also get reproducible CI basically for free. The bad is that in some repsect you already have versioning with rustup and rust-toolchain.toml. I guess the ugly are the .nix files..

Nix or not, generally I feel having a formatter and linter on such a quickly growing project is a good idea! CI should make sure everything is formatted/linted correctly before merge. CI should not rely on commands such as cargo batch which AFAIK are custom, not used anywhere else and not documented.

The CI should be open source, clearly showcasing how to build the project, or at least the part of it you can, e.g., making sure the code builds, tests get run, examples built (obviously you cannot do hw-in-the-loop if using Github Runners), et cetera :-)

@Dirbaio
Copy link
Member

Dirbaio commented May 13, 2024

unfortunately I don't think we should merge this:

  • The upside is limited: The majority of users don't use nix, this only benefits the users who do. No maintainers currently use Nix AFAIK.
  • The downside is more widespread: every maintainer and contributor needs to keep the nix files up to date for every change they do, and learn enough Nix to be able to do so.
  • I don't think it's a good tool for CI. The workflow you added takes 11min with cold cache, 1min33s with warm cache. All it does is a format check (as far as I can tell). As a comparison, the existing rustfmt check takes <5s.

using Github Actions or another open platform.

github actions is not open source! the CI system we're using is.

Reproducible builds and caching: While not implemented yet, this PR opens the ability to build embassy project reproducibly manner.

This is a library. People consume it in source form, building it themselves together with their project. We're not producing any binary artifact that people are downloading that we'd care about reproducibility. Rust's .rlib format is highly unstable.

CI should not rely on commands such as cargo batch which AFAIK are custom, not used anywhere else and not documented.

cargo batch exists because it makes CI massively faster: on a ryzen 9 7950x, running the cargo batch in ci.sh takes 2min25s, changing it to run cargo build N times takes 12min. The reason is cargo build can't share the target dir, so it can't be parallelized. Separate target dirs can but then you're building the deps over and over again which is even slower. cargo batch parallelizes and shares the target dir.

It's easy to dismiss "custom, not used anywhere else" things as something intrinsically bad. However, most projects don't have a crate with 1500 Cargo features that needs testing, so here it makes sense to use something not used anywhere else. Same for bender and HIL testing.

@multivac61
Copy link
Author

No problem at all. I understand where you are coming from. In the end you share the maintenance burden and have to understand the tools and technologies 🤗

@multivac61 multivac61 closed this May 13, 2024
@multivac61 multivac61 deleted the ci-devshell branch May 13, 2024 19:42
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