Re: [pbs-devel] [PATCH v2 proxmox-backup 2/2] status: use Option on avail/used datastore attrs

[Gabriel Goller <g.goller@proxmox.com>]

On 12/14/23 08:55, Thomas Lamprecht wrote:
> Am 11/12/2023 um 09:59 schrieb Gabriel Goller:
>> [..]
> Talked a bit with Dietmar off-list and checked this out a bit closer, having
> to check each property separately seems like slightly annoying extra work we
> might be able to avoid while not loosing anything.
>
> I.e., what about keeping this as is and instead add a new usage property that
> is an option of a struct with all three values as u64?
>
> We get this info from file-system level, so if we cannot get all at once it'd
> be rather wrong anyway (from both our access control system and how we gather
> those data from the kernel), so I think this is an all or nothing.
>
> We could then can mark the other three i64 properties as deprecated and remove
> them with the next major version, thus having a clean compatibility cut and a
> easier to use API.
I like this idea.
> Not directly related, but while at it we could also improve how such things are
> marked as "to be removed at version X).
> In PVE we use FIXME comments with a (roughly) common style, but that is naturally
> error-prone, but here we have a full compilation step, so we could add a warning
> that is only emitted if the crate version is >= next-major version (and possibly
> some "future-deprecation feature to more easily check for them manually), and thus
> find all of those occurrences easily.
Hmm I looked into this a little bit and wrote a quick macro inspired by this
crate [0]. It's basically a `proc_macro_attribute` that takes a specific 
semver condition
and gets the current crate version using the `CARGO_PKG_VERSION` 
environment variable.
Then if the version matches, it adds a `#[deprecated]` attribute or 
panics (which produces
a compiler error). For example:

#[proxmox_api_macro::deprecate_from(remove = ">= 4.x.x", note = "remove 
total and use total_new")]
pub struct DataStoreStatusListItem {
     pub total: i64,
     pub total_new: Option<u64>,
}

This won't change anything currently, but will deprecate the struct with 
the specified
message when the version hits `>=4.x.x`.

This would be perfect, but it has two problems:
  1) This is a `proc_macro_attribute`, so it can only be set on the 
struct/function and not on a
     specific field. I don't think this is possible to implement using a 
derive macro (although
     please correct me if I'm wrong) and using a function-style macro 
would be kinda ugly, cause we would
     have to write (If this is even possible):
     deprecate_from!(pub total: i64, remove=">=4.x.x")
     Although I don't think this is necessarily a deal breaker, cause we 
can use this to simply trigger
     the deprecation notice + nearly always we have to change multiple 
fields in the struct to remove a field.

  2) The bigger problem is that we (AFAIK) can't get the workspace 
package version inside the macro. `CARGO_PKG_VERSION`
     only returns the version of the crate where the invocation is in. 
For example for the example above, the version is
     `0.1.0`, the version of `pbs-api-types`.

Let me know what you think!

[0]: https://crates.io/crates/deprecate_until