CCing the Change's co-owners: @gotmax23 @fale @alexsaezm @mikelo2

Should the legacy guidelines include a preamble saying they are legacy and defining when they should/may be followed?

I would remove the word "simply" here and in the next paragraph. When reading it, it makes me feel like I should have already known this -- but that's not true. IMHO it adds no value.

So the normal procedure for "what to do if a license file is missing" does not apply here?

Packagers SHOULD use the Go Vendor Tools macros...

The previous sentence refers to commands and macros. The next sentence speaks about a command or a macro. I find the word macros in this guideline confusing -- is it meant to be explicitly mentioned the way it is?

Packages MUST include a +go-vendor-tools.toml+ file ... At least, all packages MUST have a minimal +go-vendor-tools.toml+...

This is rather complex to parse for me. Would it be possible to merge this into a single MUST?

s/generate generate/generate/

Should - %{S:2} be the default to avoid boilerplate?

On first glance, it is not obvious to me why %license vendor/modules.txt is not included in %{go_vendor_license_filelist}.

To someone who might need to package a Go application but who is otherwise unfamiliar with the Go ecosystem, the Go modules mode section is absolutely meaningless. (Speaking about myself here.) What is it? Why do I care? This was a WTF moment for me.

Could we get the %go_vendor_license_buildrequires macro to generate a dependency on go-vendor-tools so the packagers are not required to do both? (See e.g. pyproject-srpm-macros.)

I also noticed %goprep is undefined in the proposed guidelines.

Reading the two example specfiles, I don't like that the go packages are so tightly coupled with forgemeta. It seems it's possible without them but then you don't get %goprep and %{gobuilddir}, right? Let me propose an theoretical example specfile. Would that work for you? Comments added be me to explain my thoughts, not to be part of the guidelines.

%bcond check 1
Name:           ov
Version:        0.43.0
Release:        %autorelease
Summary:        Feature-rich terminal-based text viewer
# Generated by go-vendor-tools
License:        Apache-2.0 AND BSD-3-Clause AND MIT AND MPL-2.0
URL:            https://github.com/noborus/ov
Source0:        %{url}/archive/v%{version}/ov-%{version}.tar.gz
# Generated by go-vendor-tools
Source1:        ov-%{version}-vendor.tar.bz2
Source2:        go-vendor-tools.toml
ExclusiveArch:  %{golang_arches_future}
BuildRequires:  go-rpm-macros
%description
Feature-rich terminal-based text viewer. It is a so-called terminal pager.
%prep
# Unpack upstream sources (Source0) and apply patches if they exist.
# This also creates the %%{gobuilddir} directory used to store the binary built
# during %%build and unpacks the vendor archive in Source1 by default.
%goprep -p1
%generate_buildrequires
# Install license scanner dependencies incl. go-vendor-tools. By default go-vendor-tools.toml is assumed in Source2.
%go_vendor_license_buildrequires
%build
# Enable Go modules mode as required by the Guidelines.
%global gomodulesmode GO111MODULE=on
# Set version in binary. The exact value to pass to -X differs by project.
export GO_LDFLAGS="-X main.Version=%{version}"
# Build the binary
%gobuild -o %{gobuilddir}/bin/ov %{goipath}
# Generate shell completions
%{gobuilddir}/bin/ov --completion bash > %{name}.bash
%{gobuilddir}/bin/ov --completion fish > %{name}.fish
%{gobuilddir}/bin/ov --completion zsh  > %{name}.zsh
%install
# Install license files, by default go-vendor-tools.toml is assumed in Source2.
%go_vendor_license_install
# Install binaries built during %%build (the install options are not checked for correctness)
install -Dp %{gobuilddir}/bin/ov -t %{buildroot}%{_bindir}
# Install shell completions generated during %%build
install -Dpm 0644 %{name}.bash %{buildroot}%{bash_completions_dir}/%{name}
install -Dpm 0644 %{name}.fish %{buildroot}%{fish_completions_dir}/%{name}.fish
install -Dpm 0644 %{name}.zsh  %{buildroot}%{zsh_completions_dir}/_%{name}
%check
# Perform license check, by default go-vendor-tools.toml is assumed in Source2.
%go_vendor_license_check
# Run Go unit tests
%if %{with check}
%gocheck2
%endif
%files -f %{go_vendor_license_filelist}
# modules.txt used to generate automatic bundled() Provides included in %%{go_vendor_license_filelist}
%doc README.md
%{_bindir}/ov
%{bash_completions_dir}/%{name}
%{fish_completions_dir}/%{name}.fish
%{zsh_completions_dir}/_%{name}
%changelog
%autochangelog

I've replied to the first set of easy fixes. More to come.

Should the legacy guidelines include a preamble saying they are legacy and defining when they should/may be followed?

New packages should never use them and old packages should migrate over to the new guidelines as soon as possible because we want to remove the unmaintained library packages. So they really should not be used at all anymore but ~1500 packages still use the old guidelines so I didn't want to remove them yet. If you have suggestions for how to word that, feel free.

I would remove the word "simply" here and in the next paragraph. When reading it, it makes me feel like I should have already known this -- but that's not true. IMHO it adds no value.

Done.

So the normal procedure for "what to do if a license file is missing" does not apply here?

Does your comment still apply after reading === In case of missing licenses?

Packages MUST include a +go-vendor-tools.toml+ file ... At least, all packages MUST have a minimal +go-vendor-tools.toml+...

This is rather complex to parse for me. Would it be possible to merge this into a single MUST?

See the fixup commit.

s/generate generate/generate/

Fixed.

So they really should not be used at all anymore but ~1500 packages still use the old guidelines so I didn't want to remove them yet. If you have suggestions for how to word that, feel free.

See the Importnat notie at the top of https://docs.fedoraproject.org/en-US/packaging-guidelines/Python_201x/ together with the note at the beginning of https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/

Does your comment still apply after reading === In case of missing licenses?

I guess not. I am still a bit conflicted but I guess the packager could use the post_commands to generate the LICENSE file placeholder...

Fixup commits ack.

Thanks for the feedback!

Packagers SHOULD use the Go Vendor Tools macros...

The previous sentence refers to commands and macros. The next sentence speaks about a command or a macro. I find the word macros in this guideline confusing -- is it meant to be explicitly mentioned the way it is?

I changed the wording. Does it look better now?

Should - %{S:2} be the default to avoid boilerplate?

Probably not. It feels too magical, and I don't want to require that packagers have Sources listed in some specific order or have extra help and error messages in Go Vendor Tools when it can't read/parse Source2 because it doesn't exist or is some other file.

On first glance, it is not obvious to me why %license vendor/modules.txt is not included in %{go_vendor_license_filelist}.

Yeah, I wanted to do that but didn't got around to it. Currently, having %{go_vendor_license_filelist} and %license vendor/modules.txt together results in a /usr/share/license/NAME listed twice warning which isn't so nice but is harmless. Do you consider this a blocker for the guidelines or a "nice to have"? I guess it would be relatively trivial to implement...

To someone who might need to package a Go application but who is otherwise unfamiliar with the Go ecosystem, the Go modules mode section is absolutely meaningless. (Speaking about myself here.) What is it? Why do I care? This was a WTF moment for me.

Go modules are described at https://go.dev/ref/mod. It's the system Go uses to manage dependencies. Before modules were implemented, there was a different system (referred to as $GOPATH mode in the upstream docs) with different semantics that had no concept of versions or metadata and just used the filesystem. For example, the package github.com/docker/docker would be located in $GOPATH/src/github.com/docker/docker. We used to rely on the old system by disabling modules mode in order to implement a global package registry. Because of that, %gobuild always disabled modules mode.

Some projects now use Go modules features that make them incompatible with the setup expected by GOPATH mode. Others still work because the old way still supported the vendor directory, so they build, but are missing metadata (i.e., go version -m /usr/bin/foo doesn't show the dependencies used to build the package) and may break in other ways (e.g., the embeded files feature sometimes breaks). So new packages should enable modules mode to avoid all this.

I don't think we should explain all these details in the guidelines, but we need to mention it since go modules mode will need to be opt-in if we want to avoid breaking all the packages using un-vendored dependencies that relied on it.

Would linking to https://go.dev/ref/mod when mentioning Go modules (so it's clearer that it's not talking about the doomed RPM Modularity initiative, for example) make this clearer?

I also noticed %goprep is undefined in the proposed guidelines.

The guidelines mention the go-rpm-macros forge macro wrappers which include %goprep.

Reading the two example specfiles, I don't like that the go packages are so tightly coupled with forgemeta. It seems it's possible without them but then you don't get %goprep and %{gobuilddir}, right?

Pretty much, although neither is particularly important. %goprep only makes sense if you're using the forge macro wrappers to unpack sources. It's also sets up a local $GOPATH tree with symlinks that just gets ignored when modules mode is enabled. %{gobuilddir} is just an intermediate build directory that's not particularly important either.

# Unpack upstream sources (Source0) and apply patches if they exist.
# This also creates the %%{gobuilddir} directory used to store the binary built
# during %%build and unpacks the vendor archive in Source1 by default.
%goprep -p1

This would probably be best suited for a new %go_vendor_prep macro part of go-vendor-tools if it's going to have completely different behavior than the existing %goprep and not read the macros set by %gometa. Also, I'm concerned that it's doing too much by unpacking two archives and creating a directory, but I guess it'd work. How strongly do you feel about this?

So they really should not be used at all anymore but ~1500 packages still use the old guidelines so I didn't want to remove them yet. If you have suggestions for how to word that, feel free.

See the Importnat notie at the top of https://docs.fedoraproject.org/en-US/packaging-guidelines/Python_201x/ together with the note at the beginning of https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/

Thanks. I copied that.

Does your comment still apply after reading === In case of missing licenses?

I guess not. I am still a bit conflicted but I guess the packager could use the post_commands to generate the LICENSE file placeholder...

Packages could also ignore the check entirely which also isn't allowed.

Fixup commits ack.

:thumbsup:

On first glance, it is not obvious to me why %license vendor/modules.txt is not included in %{go_vendor_license_filelist}.

https://gitlab.com/fedora/sigs/go/go-vendor-tools/-/merge_requests/72

Thanks for the feedback!

Packagers SHOULD use the Go Vendor Tools macros...

The previous sentence refers to commands and macros. The next sentence speaks about a command or a macro. I find the word macros in this guideline confusing -- is it meant to be explicitly mentioned the way it is?

I changed the wording. Does it look better now?

Yes, thanks.

Should - %{S:2} be the default to avoid boilerplate?

Probably not. It feels too magical, and I don't want to require that packagers have Sources listed in some specific order or have extra help and error messages in Go Vendor Tools when it can't read/parse Source2 because it doesn't exist or is some other file.

Understood. I still think that it would be a very helpful default, but I won't fight you on that. My thoughts for reference: The -c %{S:2} argument is passed to 3 different macros and for the majority of packages, it will be Source 2.

On first glance, it is not obvious to me why %license vendor/modules.txt is not included in %{go_vendor_license_filelist}.

Yeah, I wanted to do that but didn't got around to it. Currently, having %{go_vendor_license_filelist} and %license vendor/modules.txt together results in a /usr/share/license/NAME listed twice warning which isn't so nice but is harmless. Do you consider this a blocker for the guidelines or a "nice to have"? I guess it would be relatively trivial to implement...

Not a blocker, just found it confusing. It looks like it indeed is fairly easy to do, so let's.

To someone who might need to package a Go application but who is otherwise unfamiliar with the Go ecosystem, the Go modules mode section is absolutely meaningless. (Speaking about myself here.) What is it? Why do I care? This was a WTF moment for me.

Go modules are described at https://go.dev/ref/mod. It's the system Go uses to manage dependencies. Before modules were implemented, there was a different system (referred to as $GOPATH mode in the upstream docs) with different semantics that had no concept of versions or metadata and just used the filesystem. For example, the package github.com/docker/docker would be located in $GOPATH/src/github.com/docker/docker. We used to rely on the old system by disabling modules mode in order to implement a global package registry. Because of that, %gobuild always disabled modules mode.

Some projects now use Go modules features that make them incompatible with the setup expected by GOPATH mode. Others still work because the old way still supported the vendor directory, so they build, but are missing metadata (i.e., go version -m /usr/bin/foo doesn't show the dependencies used to build the package) and may break in other ways (e.g., the embeded files feature sometimes breaks). So new packages should enable modules mode to avoid all this.

I don't think we should explain all these details in the guidelines, but we need to mention it since go modules mode will need to be opt-in if we want to avoid breaking all the packages using un-vendored dependencies that relied on it.

Would linking to https://go.dev/ref/mod when mentioning Go modules (so it's clearer that it's not talking about the doomed RPM Modularity initiative, for example) make this clearer?

It would. And I just read the addition.

Yet it makes me think: If we:

• don't want to change the default %gobuild behavior due to backward compatibility concerns
• want (close to) all packages with vendored go depndnecies to use a new mode

Wouldn't it make sense to introduce %gobuild_vendored (or similar) with the new default?

I also noticed %goprep is undefined in the proposed guidelines.

The guidelines mention the go-rpm-macros forge macro wrappers which include %goprep.

However, it doesn't really explain how to use it or when and why.

Reading the two example specfiles, I don't like that the go packages are so tightly coupled with forgemeta. It seems it's possible without them but then you don't get %goprep and %{gobuilddir}, right?

Pretty much, although neither is particularly important. %goprep only makes sense if you're using the forge macro wrappers to unpack sources.

Ack, that was not obvious to me.

It's also sets up a local $GOPATH tree with symlinks that just gets ignored when modules mode is enabled. %{gobuilddir} is just an intermediate build directory that's not particularly important either.

I see.

```

Unpack upstream sources (Source0) and apply patches if they exist.

This also creates the %%{gobuilddir} directory used to store the binary built

during %%build and unpacks the vendor archive in Source1 by default.

%goprep -p1
```

This would probably be best suited for a new %go_vendor_prep macro part of go-vendor-tools if it's going to have completely different behavior than the existing %goprep and not read the macros set by %gometa.

Let's do that then? Again, not a blocker, but I feel it would make the usage much simpler. If you create a paradigm of copy-pasting cryptic %setup -q -T -D -a1 to new packages, it would make it harder to change that later. If you cover it up with some duct tape (macros), you can improve the implementation later if needed.

Also, I'm concerned that it's doing too much by unpacking two archives and creating a directory, but I guess it'd work. How strongly do you feel about this?

Consider my feedback on this as suggestions. I won't be the one packaging Go apps with bundled dependencies for Fedora. At the end, it depends on what the Go packagers want, not me :)

Quick question:

%setup -q -T -D -a1

Is it allowed to just call tar -xf {S:1} after %autosetup / %goprep? That's a lot shorter and easier to pare than the arcane %setup invocation that has the same result. There's also %__rpmuncompress -x, but I'm not sure if that's considered internal, and it's not available on EPEL 9.

And thank you for the other suggestions about the macros! It's given me some things to think about, and it's really helpful to have another set of eyes on this. I'll get back to it soon.

Is it allowed to just call tar -xf {S:1}

It's not forbidden :)

Okay, I've implemented the %go_vendor_license_install modules.txt change and created upstream issues to track the rest of the proposed changes. I don't see them as major blockers and the changes can be made in a backward-compatible way. I'd like to get the current version of the Guidelines out the door so we can have proper Guidelines backing the new tooling and get rid of https://pagure.io/fesco/issue/3447.

Should - %{S:2} be the default to avoid boilerplate?

Probably not. It feels too magical, and I don't want to require that packagers have Sources listed in some specific order or have extra help and error messages in Go Vendor Tools when it can't read/parse Source2 because it doesn't exist or is some other file.

Understood. I still think that it would be a very helpful default, but I won't fight you on that. My thoughts for reference: The -c %{S:2} argument is passed to 3 different macros and for the majority of packages, it will be Source 2.

I opened https://gitlab.com/fedora/sigs/go/go-vendor-tools/-/issues/89. I mentioned in the issue that I think we can add another check to avoid my main concern with adding this behavior.

On first glance, it is not obvious to me why %license vendor/modules.txt is not included in %{go_vendor_license_filelist}.

Yeah, I wanted to do that but didn't got around to it. Currently, having %{go_vendor_license_filelist} and %license vendor/modules.txt together results in a /usr/share/license/NAME listed twice warning which isn't so nice but is harmless. Do you consider this a blocker for the guidelines or a "nice to have"? I guess it would be relatively trivial to implement...

Not a blocker, just found it confusing. It looks like it indeed is fairly easy to do, so let's.

https://gitlab.com/fedora/sigs/go/go-vendor-tools/-/merge_requests/72 has been merged, and I've updated the Guidelines accordingly. Thanks for the suggestion!

Yet it makes me think: If we:

• don't want to change the default %gobuild behavior due to backward compatibility concerns
• want (close to) all packages with vendored go depndnecies to use a new mode

Wouldn't it make sense to introduce %gobuild_vendored (or similar) with the new default?

I opened https://gitlab.com/fedora/sigs/go/go-vendor-tools/-/issues/90 and added some more thoughts there.

I also noticed %goprep is undefined in the proposed guidelines.

The guidelines mention the go-rpm-macros forge macro wrappers which include %goprep.

However, it doesn't really explain how to use it or when and why.

I added another sentence about the forge wrapper macros, but I'd really like to keep an extensive explanation of the forge macros out of the Guidelines. I opened https://gitlab.com/fedora/sigs/go/go2rpm/-/issues/60 to track getting rid of them entirely and removing the go2rpm specfile generator's reliance on them. Once that's done, we can update the Guidelines.

```

Unpack upstream sources (Source0) and apply patches if they exist.

This also creates the %%{gobuilddir} directory used to store the binary built

during %%build and unpacks the vendor archive in Source1 by default.

%goprep -p1
```

This would probably be best suited for a new %go_vendor_prep macro part of go-vendor-tools if it's going to have completely different behavior than the existing %goprep and not read the macros set by %gometa.

Let's do that then? Again, not a blocker, but I feel it would make the usage much simpler. If you create a paradigm of copy-pasting cryptic %setup -q -T -D -a1 to new packages, it would make it harder to change that later. If you cover it up with some duct tape (macros), you can improve the implementation later if needed.

Also, I'm concerned that it's doing too much by unpacking two archives and creating a directory, but I guess it'd work. How strongly do you feel about this?

This one I'll probably nack for now :). I replaced the cryptic %setup invocation with tar -xf which should address the immediate concern. I'll give this more thought as part of the https://gitlab.com/fedora/sigs/go/go2rpm/-/issues/60, but I'd like to avoid adding another %fooprep macro for now.

I'm keeping this in draft state because it needs to be squashed before it's merged, but please consider this ready other than that.

OK, thanks for opening the issues. I am happy to approve this as is, with a possibility of future improvements.

+1

+1 as well.

+1 ... if you push the squashed commit I'll merge it (although there's no requirement to squash, if you'd rather not).

Minor nit, but the Change linked here says that vendoring is the default, not required as a MUST implies.

Hm, would be good to get that amended, I agree.