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

[michalfita]

We're probably only user of svd2rust as library on the planet. However, since this appeared we're not only forced into ugly assignment of individual fields into mutable (like in atsams-rs/atsamx7x-pac#7), but we're stripped from builds failing on new configuration parameters we should consider.

What is the rationale behind prevention of use of struct expressions to instantiate a new config::Config?

[burrbull]

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

[Emilgardis]

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.

[jessebraham]

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().

[burrbull]

@jessebraham What about "builder" methods? https://github.com/rust-embedded/svd/blob/f94bed92ba32de783138ac2166f24e6a44585755/svd-parser/src/lib.rs#L54

[Emilgardis]

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

[michalfita]

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.

[Emilgardis]

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());

[michalfita]

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.