tosc-rs / mnemos

An Operating System for Building Small Computers
https://mnemos.dev
Apache License 2.0
234 stars 14 forks source link

Add that `mn` requires bindeps #275

Closed LegNeato closed 10 months ago

LegNeato commented 10 months ago

Without this installing errors with:

error: failed to parse manifest at `mnemos/tools/manganese/Cargo.toml`

Caused by:
  `artifact = …` requires `-Z bindeps` (cargo-binutils)
hawkw commented 10 months ago

Ah, I had not really considered the use case of installing mn using cargo install. Instead, I had assumed that it would always be run using the cargo mn alias defined in the workspace's .cargo/config.toml, which will always work fine because that file also explicitly enables the bindeps unstable feature.

I'm not sure whether or not we should include this in the README. On one hand, it does tell users how to avoid problems when installing mn using cargo install. On the other hand, I'm not actually sure if installing mn that way is a good idea. The advantage of only ever running it using cargo run is that it is potentially rebuilt whenever the Cargo.lock entries for its dependencies changes. This means that if you run mn using the alias that calls cargo run, you get whatever versions of the bindeps are currently in the lockfile. If those haven't changed, mn and its dependencies are not recompiled every time its run, but if the lockfile has changed, the cargo run command will build new versions of the bindeps. This ensures that everyone gets compatible versions of those dependencies whenever they use cargo mn.

On the other hand, if mn is cargo installed, it's built once, and every subsequent time you run cargo mn, you get the same thing, regardless of the mnemos repo's Cargo.lock. This is fine with regards to the actual mn binary, since it's very simple and won't change often. However, it also means that the versions of the bindeps that are downloaded and compiled are the versions you get when mn is cargo installed, and if the lockfile updates those dependencies, you still get the old ones. This means that if we later update the lockfile to depend on new versions of those tools, any contributors who have cargo installed mn will see their versions of the build dependencies begin to drift from what other contributors are using, potentially introducing compatibility issues with the various scripts run using mn.

Because of the version drift issue, I think it might be better to just have the README suggest that mn should not be cargo installed, and that it should instead always be used through cargo run --package manganese , or through the cargo mn alias defined in the workspace, which expands to that. What do you think?

LegNeato commented 10 months ago

Ok. I didn't actually know how to run it and assumed it was a normal cargo tool. I didn't know you could define aliases in the workspace, and I work with a lot of different rust projects so this probably will be a tripping point for folks. Thanks for the quick response!