Does config::Config really have to be #[non_exhaustive]? #925

Activity

1. burrbull commented on Mar 20, 2025

   

   burrbull

   on Mar 20, 2025

   Member

   More actions

   We're probably only user of svd2rust as library on the planet.

   You are not. See https://github.com/esp-rs/esp-pacs

   Does config::Config really have to be #[non_exhaustive]?

   In other case we need to make major release each time new option is added. Although we still have many major releases.

   Any suggestion on improving config/API part of svd2rust is appreciated.

   cc @Emilgardis
2. Emilgardis commented on Mar 20, 2025

   

   Emilgardis

   on Mar 20, 2025

   Member

   More actions

   the reason for this is to introduce new fields without them being major semver breaks.
   I do wish that Config {..Default::default()} was possible though, but it's not by design :/

   I'd recommend using the serde Serialize impl to fill the fields.
3. jessebraham commented on Mar 21, 2025

   

   jessebraham

   on Mar 21, 2025

   Member

   More actions

   In other case we need to make major release each time new option is added. Although we still have many major releases.

   Not sure what you mean by this. When the major version is 0, which is the case for svd2rust, semver makes no stability guarantees, minor releases are free to break APIs as they please:

   4. Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

   I also would prefer to be able to use Default::default().
4. burrbull commented on Mar 21, 2025

   

   burrbull

   on Mar 21, 2025

   Member

   More actions

   @jessebraham What about "builder" methods? https://github.com/rust-embedded/svd/blob/f94bed92ba32de783138ac2166f24e6a44585755/svd-parser/src/lib.rs#L54
5. Emilgardis commented on Mar 21, 2025

   

   Emilgardis

   on Mar 21, 2025

   Last edited by Emilgardis

   Member

   More actions

   In other case we need to make major release each time new option is added. Although we still have many major releases.

   Not sure what you mean by this. When the major version is 0, which is the case for svd2rust, semver makes no stability guarantees, minor releases are free to break APIs as they please:

   4. Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

   I also would prefer to be able to use Default::default().

   That may be true with semver, but cargo goes by a modified definition. The leftmost non-zero version component is the major version number.
   See https://doc.rust-lang.org/cargo/reference/semver.html

   This guide uses the terms “major” and “minor” assuming this relates to a “1.0.0” release or later. Initial development releases starting with “0.y.z” can treat changes in “y” as a major release, and “z” as a minor release. “0.0.z” releases are always major changes. This is because Cargo uses the convention that only changes in the left-most non-zero component are considered incompatible.

   So no, we are not free to do any api breaks in 0.x.y without incrementing x
6. michalfita commented on Mar 22, 2025

   

   michalfita

   on Mar 22, 2025

   Author

   More actions

   I'd recommend using the serde Serialize impl to fill the fields.

   @Emilgardis Serialize? Into what? Using serde means I need build dependency on serde and complicate my build.rs.

   the reason for this is to introduce new fields without them being major semver breaks.
   I do wish that Config {..Default::default()} was possible though, but it's not by design :/

   What about adding a field experimental to Config and under normal circumstances keep it Default. Then promote configuration options from there to config::Config only with major releases? Then, we - the PAC crate maintainers - worry only about important changes, but ignore new options until new major goes out.

   For me, really anything other struct expression in build.rs is tedious and prone to mistakes in future. I'm sure ESP's with their xtask thing actually feels similar to me.

   Any suggestion on improving config/API part of svd2rust is appreciated.

   Hopefully, my suggestion above is worth considering.
7. Emilgardis commented on Mar 22, 2025

   

   Emilgardis

   on Mar 22, 2025

   Member

   More actions

   I have a proposal, not solving this particular situation but in general.

   It's sometimes better to not use build.rs and instead provide the generated code with the source-code. This way you'll have no build-time dependency on svd2rust at all ;)

   You can even have tests that checks that the generated code is up-to-date

   What about adding a field experimental to Config and under normal circumstances keep it Default. Then promote configuration options from there to config::Config only with major releases? Then, we - the PAC crate maintainers - worry only about important changes, but ignore new options until new major goes out.

   I think I like this alternative!

   as @burrbull mentioned, what about if we provide more builder methods?

   let config = Config::default()
       .make_mod(true)
       // etc
       .ident_formats(svd2rust::config::IdentFormats::legacy_theme());
8. michalfita commented on Mar 24, 2025

   

   michalfita

   on Mar 24, 2025

   Author

   More actions

   It's sometimes better to not use build.rs and instead provide the generated code with the source-code. This way you'll have no build-time dependency on svd2rust at all ;)

   We had this. With 51 PACs hundreds of files each, keeping the code on the repo cause GitHub to kneel down, especially in PRs. Alternative is to use CI/CD to generate the source code just before the release to crates.io, but so far I don't have any CI/CD for these PACs as none of solutions I tried world, so I have to handcraft something. One day maybe...

   To this day original repo takes very long to clone.

   Builder would be nice, but still leaves us unaware of new options if the build doesn't fail (which is why #[non_exhaustive] is used there), but I think it's a problem of contradicting expectations here.

   👍React with 👍1Reacted by Emilgardis