Thank you for working on this!

One thing that strikes me as slightly odd (or potentially problematic) is that you define %{version} twice (once by setting %global version, once by setting the Version: %{forgeversion} tag). It might work, but only due to a quirk of how RPM parses spec files. It might be better to use a different name for the macro rather than relying on the order in which %global version foo and Version: bar are evaluated.

Agreed with @decathorpe. I haven't looked at the macro definition for %forgeversion yet, but based on these examples i don't really understand — semantically — the purpose of how it works.

Does the %forgeversion macro depend on a global %version variable having being defined, which it will then consume? That seems like a recipe for confusion, if not disaster.

If %forgeversion is just used to append snapshot info, why not use it that way?

Version:     0.37.4%{?forgeversion}

(I'd even consider calling it something else, like %forgesnap or %forgedist.)

If the version number absolutely needs to be accessible to the macro, then why not name the variable that holds it something else?

%global pkgversion 0.37.4
Version: %forgeversion

(Either way) %version will get defined as the entire version string including the forge additions, but that's how it's supposed to work in rpm specfile code, and why other variables are often used to hold version numbers for use in things like download URLs and etc. when necessary.

Yeah, now that I've looked at both the current redhat-rpm-config-sourced macros.forge and the updated one at https://git.sr.ht/~gotmax23/forge-srpm-macros/tree/main/item/rpm/macros.d/macros.forge, I have to say that this magical (ab)use of a pre-defined global %version variable seems like a terrible idea.

The comments at the top of the file (both old and new) even say,

#   %{version<number>}   the packaged version
#                          – %{version}/%{version0} are set via:
#                              Version:
#                          – because git is lacking a built-in version
#                            reference, %{version<number>} will be translated
#                            into %{tag<number>} using unreliable heuristics;
#                            set %{tag<number>} directly if those fail

But the new code is trying to make that "set via: Version:" part no longer true, which seems like an extremely ill-advised perversion of how rpmbuild specfile processing works. This probably isn't the correct place to continue this, so I'll open an issue at https://todo.sr.ht/~gotmax23/forge-srpm-macros

I'll open an issue at https://todo.sr.ht/~gotmax23/forge-srpm-macros

Oh. Apparently I... can't do that.

Well, in addition to what I wrote above, you should also take a look at https://docs.pagure.org/fedora-infra.rpmautospec/peculiarities.html#peculiarities if you haven't already. In particular:

Package versions must be determinable from the spec file alone

Both the Koji plugin and fedpkg preprocess package spec files outside of the target build root. If the version field of a package depends on macros not defined in the spec file (directly or indirectly), this will likely result in unexpected behavior if the macros in question differ between the environment of the target and that where preprocessing happens.

...The versioning done by the forge macros may not be compatible with the new-style versioning, despite its status as the recommended approach. That's why legacy versioning is still supported for packages that have "complex version requirements", which seems to describe any forge-driven package.

There's even support in the %autorelease macro to specify additional version elements as part of the Release: field

One thing that strikes me as slightly odd (or potentially problematic) is that you define %{version} twice (once by setting %global version, once by setting the Version: %{forgeversion} tag). It might work, but only due to a quirk of how RPM parses spec files. It might be better to use a different name for the macro rather than relying on the order in which %global version foo and Version: bar are evaluated.

We could change this to suggest using %version0 instead. That's already supported by %forgemeta for reading to determine %forgesource. What do you think?

If %forgeversion is just used to append snapshot info, why not use it that way?

%forgemeta needs to know the base version of software to determine %forgesource. That's why %global version is set before the %forgemeta / %gometa invocation and then Version: %{forgeversion} is set after. %{forgeversion} contains %{version} + any snapshot information determined by %forgemeta.

If the version number absolutely needs to be accessible to the macro, then why not name the variable that holds it something else?

(Note: Some may not know this, but the forge macros support multiple %forgeurl definitions (e.g. %forgeurl1, %forgeurl2) to define multiple sources. Therefore, %version<suffix>, %commit<suffix, etc.) can be specified to correspond to each %forgeurl<suffix> block.)

We could change it to something else, but if that thing isn't defined, then the forge macros MUST fall back to reading the value from %version / %version<suffix> (whether that's set with %global version or through the Version: tag) to preserve backwards compatibility. I'm suggesting that we change %global version everywhere in this patch to %global version0, as this will require just a single code change in %forgeversion itself, but %forgemeta and %forgesource will remain the same without having to add extra fall backs and code branches. If that's undesired, I could add support for a %baseversion macro that'll behave the same as %versionand %version<suffix> currently do for the purposes of determining %forgesource.

I'll open an issue at https://todo.sr.ht/~gotmax23/forge-srpm-macros

Oh. Apparently I... can't do that.

See https://git.sr.ht/~gotmax23/forge-srpm-macros/tree/main/item/CONTRIBUTING.md#issue-reporting-and-feature-requests. We might as well continue the discussion here, though.

Both the Koji plugin and fedpkg preprocess package spec files outside of the target build root. If the version field of a package depends on macros not defined in the spec file (directly or indirectly), this will likely result in unexpected behavior if the macros in question differ between the environment of the target and that where preprocessing happens.

It really should not do that. Independent of this feature, this assumption will break if packagers start adopting any RPM features that depend on a newer version than what's available in the environment running the Koji plugin. In any case, we should wait to start recommending %{forgeversion} in the guidelines until we've backported it to stable Fedoras, as users running fedpkg srpm in rpmautospec-less packages won't be able to use this either.

The versioning done by the forge macros may not be compatible with the new-style versioning, despite its status as the recommended approach.

What makes it incompatible with new-style versioning? We're allowing packagers to opt-in to storing the same information that it'd otherwise store in %distprefix. I think this approach is better, as it makes the whole thing more explicit and removes the possibility for bugs like https://pagure.io/releng/issue/10852 that come about when %dist is redefined.

Yeah, using %{version0} for the version of the "main" package / first source URL / etc. would be better, I think. It's already supported by the forge macros and is less likely to cause confusion.

What makes it incompatible with new-style versioning?

This is likely referring to the current implementation of the forge macros without %{forgeversion}?

Yeah, using %{version0} for the version of the "main" package / first source URL / etc. would be better, I think. It's already supported by the forge macros and is less likely to cause confusion.

Done.

See https://git.sr.ht/~gotmax23/forge-srpm-macros/commit/f06ccd7dffe849a28f1859c4cc79d4764031339b and the latest commit in this PR.

This is likely referring to the current implementation of the forge macros without %{forgeversion}?

Yeah, that would make more sense :).