#1 Add the automation described in https://fedoraproject.org/wiki/More_Go_packaging
Closed 2 years ago by nim. Opened 2 years ago by nim.
rpms/ nim/go-srpm-macros more-go-packaging  into  master

file modified
+24 -6

@@ -1,14 +1,21 @@ 

  Name:           go-srpm-macros

- Version:        2

- Release:        10%{?dist}

+ Version:        3

+ Release:        1%{?dist}

  Summary:        RPM macros for building Golang packages for various architectures

  Group:          Development/Libraries

  License:        GPLv3+

  Source0:        macros.go-srpm

+ Source1:        macros.go-rpm

+ Source10:       go.attr

+ Source11:       go.prov

+ Source12:       go.req

+ 

  BuildArch:      noarch

  # for install command

  BuildRequires:  coreutils

  

+ Requires:       redhat-rpm-config >= 73

I think it is not necessary, if I understand correctly that the dependent change is already part of minimal build root in rawhide.

+ 

  %description

  The package provides macros for building projects in Go

  on various architectures.

@@ -20,13 +27,24 @@ 

  # nothing to build, just for hooks

  

  %install

- install -m 644 -D "%{SOURCE0}" \

-     '%{buildroot}%{_rpmconfigdir}/macros.d/macros.go-srpm'

- 

+ install -m 0755 -vd %{buildroot}%{_rpmconfigdir}/{macros.d,fileattrs}/

+ install -m 0644 -vp %{SOURCE0} %{SOURCE1} \

+                     %{buildroot}%{_rpmconfigdir}/macros.d/

+ install -m 0644 -vp %{SOURCE10} \

+                     %{buildroot}%{_rpmconfigdir}/fileattrs/

+ install -m 0755 -vp %{SOURCE11} %{SOURCE12} \

+                     %{buildroot}%{_rpmconfigdir}/

  %files

- %{_rpmconfigdir}/macros.d/macros.go-srpm

+ %{_rpmconfigdir}/macros.d/macros.*

+ %{_rpmconfigdir}/fileattrs/go.attr

+ %{_rpmconfigdir}/go.prov

+ %{_rpmconfigdir}/go.req

  

  %changelog

+ * Tue Jan 23 2018 Nicolas Mailhot <nim@fedoraproject.org> - 3-1

+ - Add Go autodeps

+ - https://fedoraproject.org/wiki/More_Go_packaging

+ 

  * Wed Jul 26 2017 Fedora Release Engineering <releng@fedoraproject.org> - 2-10

  - Rebuilt for https://fedoraproject.org/wiki/Fedora_27_Mass_Rebuild

  

file added
+5

@@ -0,0 +1,5 @@ 

+ %__go_provides          %{_rpmconfigdir}/go.prov --goipath "%{?goipath}" --prefix "%{buildroot}" --gopath "%{gopath}" --version "%{?epoch:%{epoch}:}%{version}-%{release}" %{?commit:-a "(commit=%{commit})"} %{?branch:-a "(branch=%{branch})"} %{?tag:-a "(tag=%{tag})"}

+ %__go_requires          %{_rpmconfigdir}/go.req  --goipath "%{?goipath}" --prefix "%{buildroot}" --gopath "%{gopath}" --version "%{?epoch:%{epoch}:}%{version}-%{release}"

+ %__go_path              ^%{gopath}/src/.+/.*$

+ %__go_magic             ^((.*, )?directory|(broken )?symbolic link to .*)$

+ %__go_flags             magic_and_path

file added
+88

@@ -0,0 +1,88 @@ 

+ #!/bin/bash

Have you considered "go list" for this script. I think that this script would be much simpler and future proof with usage of go list. And due to this better to be part of the go-compilers.

+ 

+ usage() {

+ cat >&2 << EOF_USAGE

+ Usage: $0 -g <go import path> [-p <prefix>] [-G <go path>] [-v <version string>] [-a <attributes>]

+ <go import path>   the Go import path of the target package

+ <prefix>:          an eventual prefix path such as %{buildroot}

+ <go path>:         the root of the Go source tree

+ <version string>:  tag the provides with the <version string>

+ <attributes>:      attributes to add to the provides, for example

+                    (commit=XXXX)(branch=YYYY) or

+                    (vermod=alpha1)

+ 

+ Example:

+ echo filename | $0 -g golang.org/x/net -G /usr/share/gocode -v 2.5-0.1

+ echo filename | $0 -g golang.org/x/net -a '(commit=XXXX)(branch=YYYY)'

+ echo filename | $0 -g golang.org/x/net -a '(vermod=alpha1)'

+ EOF_USAGE

+ exit 1

+ }

+ 

+ if ! options=$(getopt -n $0 -o hg:p:G:v:a: -l help,goipath:,prefix:,gopath:,version:,attr:,attribute:,attributes: -- "$@")

+ then

+     # something went wrong, getopt will put out an error message for us

+     exit 2

+ fi

+ 

+ eval set -- "$options"

+ 

+ provides() {

+   package="${1#${root}/src/}"

+   for index in "${!deco[@]}" ; do

+     echo "golang(${package})${deco[index]}${version}"

+   done

+ }

+ 

+ version=''

+ prefix=''

+ gopath=/usr/share/gocode

+ declare -A attributes

+ 

+ while [ $# -gt 0 ] ; do

+   case $1 in

+     -h|--help)    usage ;;

+     -g|--goipath) goipath="$2" ; shift;;

+     -p|--prefix)  prefix=$(realpath -sm "$2") ; shift;;

+     -G|--gopath)  gopath="$2" ; shift;;

+     -v|--version) version=" = $2" ; shift;;

+     -a|--attr|--attribute|--attributes)

+                   IFS=')' read -r -a newattrs <<< "$2"

+                   for index in "${!newattrs[@]}" ; do

+                     newattrs[index]=${newattrs[index]#\(}

+                     attributes[${newattrs[index]%%=*}]=${newattrs[index]#*=}

+                   done ; shift;;

+     (--)          shift; break;;

+     (-*)          echo "$0: error - unrecognized option $1" >&2; exit 3;;

+     (*)           break;;

+   esac

+   shift

+ done

+ 

+ [[ -z ${goipath} ]] && exit 0

+ 

+ root="${prefix}${gopath}"

+ 

+ deco=( "" )

+ # Some filtering may be needed in the future

+ for key in "${!attributes[@]}"; do

+   [ -n "${attributes[$key]}" ] && deco+=( "($key=${attributes[$key]})" )

+ done

+ 

+ while read dir ; do

Could you extend the implementation with some comments? It is not always straightforward to see what is the expected behavior.

+   if [[ -L $dir ]] ; then

+     targetdir=$(realpath -m "$dir")

+     [[ $targetdir != ${prefix}* ]] && targetdir="${prefix}${targetdir}"

+     if [[ ${targetdir}/ == ${root}/src/${goipath}/* ]] ; then

+       for subdir in $(find "$targetdir" -type d) ; do

+         subdir="${dir}${subdir#${targetdir}}"

+         provides "$subdir"

+       done

+     fi

+   else

+     if [[ -d $dir ]] ; then

+       dir=$(realpath -sm "$dir")

+       [[ ${dir}/ == ${root}/src/${goipath}/* ]] && provides "$dir"

+     fi

+   fi

+ done | sort | uniq | grep -v '/\([._]\)\?\(internal\|vendor\)\([/)]\)'

file added
+69

@@ -0,0 +1,69 @@ 

+ #!/bin/bash

Have you considered implementing this command in the compilers meta package(so you can depend on the golang compiler) and using "go list ./..." command and some output formatting, directory traversal?

+ 

+ usage() {

+ cat >&2 << EOF_USAGE

+ Usage: $0 -g <go import path> [-p <prefix>] [-G <go path>] [-v <version string>]

+ <go import path>   the Go import path of the target package

+ <prefix>:          an eventual prefix path such as %{buildroot}

+ <go path>:         the root of the Go source tree

+ <version string>:  tag symlinked renamings with <version string>

+ 

+ Example:

+ echo filename | $0 -g golang.org/x/net -G /usr/share/gocode

+ EOF_USAGE

+ exit 1

+ }

+ 

+ if ! options=$(getopt -n $0 -o hg:p:G:v: -l help,goipath:,prefix:,gopath:,version: -- "$@")

+ then

+     # something went wrong, getopt will put out an error message for us

+     exit 2

+ fi

+ 

+ eval set -- "$options"

+ 

+ 

+ prefix=''

+ gopath=/usr/share/gocode

+ 

+ while [ $# -gt 0 ] ; do

+   case $1 in

+     -h|--help)    usage ;;

+     -g|--goipath) goipath="$2" ; shift;;

+     -p|--prefix)  prefix=$(realpath -sm "$2") ; shift;;

+     -G|--gopath)  gopath="$2" ; shift;;

+     -v|--version) version=" = $2" ; shift;;

+     (--)          shift; break;;

+     (-*)          echo "$0: error - unrecognized option $1" >&2; exit 3;;

+     (*)           break;;

+   esac

+   shift

+ done

+ 

+ [[ -z ${goipath} ]] && exit 0

+ 

+ root="${prefix}${gopath}"

+ 

+ while read dir ; do

+   if [[ -L $dir ]] ; then

+     targetdir=$(realpath -m "$dir")

+     [[ $targetdir != ${prefix}* ]] && targetdir="${prefix}${targetdir}"

+     if [[ ${targetdir}/ == ${root}/src/${goipath}/* ]] ; then

+       for subdir in $(find "$targetdir" -type d) ; do

+         echo "golang(${subdir#${root}/src/})${version}"

+       done

+     fi

+   else

+     if [[ -d $dir ]] ; then

+       dir=$(realpath -sm "$dir")

+       package="${dir#${root}/src/}"

+       GOPATH=$root go list -e -f \

+             '{{ join .Imports "\n" }}' "$package" |\

+         while read import ; do

+           GOPATH=$root go list -e -f \

Oh... I think that this creates implicit dependency on go command. I think that is not a thing that we want to have in minimal build root. Looking on this I think that this script would be better part of the go-compiler package.

This will not cover cases of imported packages of generated Go files. E.g. one needs to run "make gen" in Kubernetes to generate other go files which have their own dependencies.

+             '{{if not .Standard}}{{.ImportPath | printf "golang(%s)\n"}}{{end}}' "$import"

+         done

+     fi

+   fi

+ done | sort | uniq \

+      | grep -v '/\([._]\)\?\(internal\|testdata\)\([/)]\)' | grep -v 'golang(C)'

file added
+114

@@ -0,0 +1,114 @@ 

+ # Copyright (c) 2018 Nicolas Mailhot <nim@fedoraproject.org>

+ #

+ # This file is distributed under the terms of GNU GPL license version 3, or

+ # any later version.

+ #

+ # This file contains macros needed at %%build %%install and %%check

+ # stage by Golang packages.

+ # The macros necessary at %%setup and srpm stage are in the sister file

+ # macros.go-srpm

+ 

+ # find directory filter to remove elements usually not needed to build other Go projects

+ %gofinddirfilter  -regextype egrep \! -iregex '.*/(.*[._-])?(example(s)?|test(_)?data)/.*' \! -ipath '*/vendor/*' \! -ipath '*/test cases/*'

+ 

+ # find filter to identify resources usually needed to build other Go projects

+ %gofindfilter     -regextype egrep -iregex '.*\\.(go|c|h|s|tmpl|proto|json)' \! -iname '*_test.go' %{gofinddirfilter}

+ 

+ # find filter to identify unit tests

+ %gofindtestfilter                  -iname '*.go' %{gofinddirfilter}

+ 

+ # Collect md files spread in subdirectories

+ %gocollectmd %{expand:

+ for mdfile in $(find . -iname '*.md' %{gofinddirfilter}) ; do

+   suffix=$(dirname $mdfile | sed 's+^\./++g' | sed 's+/+·+g')

+   if [[ $suffix != '.' ]] ; then

+     cp -p "$mdfile" "$(echo $(basename $mdfile) | sed 's+\.md$++g')·${suffix}.md"

+   fi

+ done}

+ 

+ # Try to install Go package files in sensible locations, with strict directory

+ # ownership as required by Go autodeps

+ #

+ # %%goinstall takes a list of files as argument.

+ #

+ # %%goinstall will generate a file list that can be used in a %%files spec

+ # section. The default file list name is devel.file-list. It can be overriden

+ # by passing the -f argument to the macro with another filename.

+ #

+ # When invoked several times it will append to existing file lists not create

+ # a new one.

+ #

+ # When invoked several times with different file list names, it will attribute

+ # directories to the first file list that makes use of them only. This is

+ # intentional, to avoid triggering Go autodeps on the same Go directory in

+ # different subpackages. Therefore, splitting code in several subpackages

+ # requires careful though about %%goinstall invocation order.

+ %goinstall(f:) %{expand:

+ %global file_list %{?-f*}%{!?-f*:devel.file-list}

+ install -m 0755 -vd "%{buildroot}%{gopath}/src"

+ for file in %* ; do

+   file="${file#./}"

+   [[ -d "$file" && ! -L "$file" ]] && srcdir="$file" || srcdir=$(dirname "$file")

+   destdir="%{buildroot}%{gopath}/src/%{goipath}/$srcdir"

+   destdir="${destdir%/.}"

+   dir="$destdir"

+   dirs=()

+   while [[ ! -e "$dir" ]] ; do

+     dirs=("$dir" "${dirs[@]}")

+     dir=$(dirname "$dir")

+   done

+   for dir in "${dirs[@]}" ; do

+     install -m 0755 -vd "$dir"

+     if $(echo "$dir" | grep -q "^%{buildroot}%{gopath}/src/%{goipath}") ; then

+       touch -r           ".${dir#%{buildroot}%{gopath}/src/%{goipath}}" "$dir"

+     fi

+     echo "%%dir \"${dir#%{buildroot}}\"" >> %{file_list}

+   done

+   [[ -L "$file" ]] && cp -vpa "$file" "$destdir/"

+   [[ -f "$file" ]] && install -m 0644 -vp  "$file" "$destdir/"

+   [[ -f "$file" || -L "$file" ]] && echo "%%{gopath}/src/%%{goipath}/$file" >> %{file_list}

+ done}

+ 

+ # Create a local Go build root

+ # Useful in %%build and %%check

+ %gobuildroot() %{expand:

+   GO_BUILD_PATH="$PWD/_build"

+   %global gobuildpath "$GO_BUILD_PATH"

+   install -m 0755 -vd "$(dirname %{gobuildpath}/src/%{goipath})"

+   ln -fs "$PWD" "%{gobuildpath}/src/%{goipath}"

+   install -m 0755 -vd _bin

+   export GOPATH="%{gobuildpath}:%{gopath}"

+   %{?commit:export LDFLAGS="-X %{goipath}/version.GitSHA=%{commit}"}

+ }

+ 

+ # Run “go test” on all subdirs except for those provided in parameters

+ # Examples:

+ #  – run all available unit tests

+ #    %gochecks

+ #  — run all available unit tests except in root

+ #    %gochecks .

+ #  — run all available unit tests except in root and foo subdir

+ #    %gochecks . foo

+ #  — run all available unit tests except in I love cake and _examples subdirectories

+ #    %gochecks 'I love cake' '_examples/*'

+ %gochecks() %{expand:

I think this could be replace with %gotest ./..., possibly by appending possibility to pass arg "-run regexp". IMHO in general there shouldn't be need to skip individual test cases, i.e. test case should check for special conditions and fail gracefully, when not met.

+ %gobuildroot

+ export PATH=$PATH:%{buildroot}%{_bindir}

+ set +x

+ for sub in $(find . %{gofindtestfilter} -exec dirname '\{\}' \\;|sort|uniq|sed s'+^./++') ; do

+   process=true

+   declare -a 'excludes=('"%*"')'

+   for exclude in "${excludes[@]}" ; do

+     [[ $sub == $exclude ]] && process=false && break

+   done

+   if [ "$process" = true ] ; then

+     echo "Testing $sub…"

+     pushd "%{gobuildpath}/src/%{goipath}/$sub"

+     set -x

+     %{gotest}

+     set +x

+     popd

+   fi

+ done

+ set -x

+ }

file modified
+129 -2

@@ -1,4 +1,6 @@ 

- # Copyright (c) 2015 Jakub Cajka <jcajka@redhat.com>, Jan Chaloupka <jchaloup@redhat.com>

+ # Copyright (c) 2015-2018 Jakub Cajka <jcajka@redhat.com>,

+ #                         Jan Chaloupka <jchaloup@redhat.com>,

+ #                         Nicolas Mailhot <nim@fedoraproject.org>

  # This file is distributed under the terms of GNU GPL license version 3, or

  # any later version.

  

@@ -6,7 +8,8 @@ 

  # with golang compiler or gcc-go compiler based on an architecture.

  # Golang is primarly for primary architectures, gcc-go for secondary.

  #

- # This file provides only macros and must not use any other package.

+ # This file provides only macros and must not use any other package except

+ # redhat-rpm-macros.

  

  # Define arches for PA and SA

  %golang_arches   %{ix86} x86_64 %{arm} aarch64 ppc64le s390x

@@ -18,3 +21,127 @@ 

  

  # Define go_compilers macro to signal go-compiler package is available

  %go_compiler     1

+ 

+ # Sanitize a Go import path that can then serve as rpm package name

+ # Mandatory parameter: a Go import path

+ %gorpmname() %{lua:

+ local goname = rpm.expand("%1")

+ goname       = string.lower("golang-" .. goname .. "/")

I'm not sure that converting the package name/import path to lowercase is always desirable. Naming guidelines doesn't forbid having packages with camel case or similar. Not saying that I don't prefer case uniformity, but we should respect(or make possible to respect) upstream naming/import.

+ goname       = string.gsub(goname, "^([^/]+)%.([^%./]+)/", "%1/")

+ goname       = string.gsub(goname, "(%d)%.(%d)",           "%1|%2")

+ goname       = string.gsub(goname, "[%._/%-]+",            "-")

+ local result = ""

+ local tokens = {}

+ tokens["go"]     = true

+ tokens["git"]    = true

+ for token in string.gmatch(goname, "[^%-]+") do

+    if not tokens[token] then

+       result = result .. "-" .. token

+       tokens[token] = true

+    end

+ end

+ result = string.gsub(result, "^-", "")

+ result = string.gsub(result, "|", ".")

+ result = string.gsub(result, "%-v([%.%d])", "%1")

+ print(result)

+ }

+ 

+ # Map Go information to rpm metadata. This macro will compute default spec

+ # variable values.

+ #

+ # The following spec variable MUST be set before calling the macro:

+ #

+ #   goipath   the packaged Go project import path

+ #

+ # The following spec variables SHOULD be set before calling the macro:

+ #

+ #   forgeurl  the project url on the forge, strongly recommended, if it can not

+ #             be deduced from goipath; alternatively, use -u <url>

+ #   Version   if applicable, set it with Version: <version>

+ #   tag       if applicable

+ #   commit    if applicable

+ #

+ # The macro will attempt to compute and set the following variables if they are

+ # not already set by the packager:

+ #

+ #   goname         an rpm-compatible package name derived from goipath

+ #   gosource       an URL that can be used as SourceX: value

+ #   gourl          an URL that can be used as URL: value

+ #

+ # It will delegate processing to the forgemeta macro for:

+ #

+ #   forgesource    an URL that can be used as SourceX: value

+ #   forgesetupargs the correct arguments to pass to %setup for this source

+ #                  used by %forgesetup and %forgeautosetup

+ #   archivename    the source archive filename, without extentions

+ #   archiveext     the source archive filename extensions, without leading dot

+ #   archiveurl     the url that can be used to download the source archive,

+ #                  without renaming

+ #   scm            the scm type, when packaging code snapshots: commits or tags

+ #

+ # If the macro is unable to parse your forgeurl value set at least archivename

+ # and archiveurl before calling it.

+ #

+ # Most of the computed variables are both overridable and optional. However,

+ # the macro WILL REDEFINE %{dist} when packaging a snapshot (commit or tag).

+ # The previous %{dist} value will be lost. Don’t call the macro if you don’t

+ # wish %{dist} to be changed.

+ #

+ # Optional parameters:

+ #   -u <url>  Ignore forgeurl even if it exists and use <url> instead. Note

+ #             that the macro will still end up setting <url> as the forgeurl

+ #             spec variable if it manages to parse it.

+ #   -s  Silently ignore problems in forgeurl, use it if it can be parsed,

+ #       ignore it otherwise.

+ #   -p  Restore problem handling, override -s.

+ #   -v  Be verbose and print every spec variable the macro sets.

+ #   -i  Print some info about the state of spec variables the macro may use or

+ #       set at the end of the processing.

+ %gometa(u:spvi) %{expand:%{lua:

+ local forgeurl    = rpm.expand("%{?-u*}")

+ if (forgeurl == "") then

+   forgeurl        = rpm.expand("%{?forgeurl}")

+ end

+ -- Be explicit about the spec variables we’re setting

+ local function explicitset(rpmvariable,value)

+   rpm.define(rpmvariable .. " " .. value)

+   if (rpm.expand("%{?-v}") ~= "") then

+     rpm.expand("%{echo:Setting %%{" .. rpmvariable .. "} = " .. value .. "}")

+   end

+ end

+ -- Never ever stomp on a spec variable the packager already set

+ local function safeset(rpmvariable,value)

+   if (rpm.expand("%{?" .. rpmvariable .. "}") == "") then

+     explicitset(rpmvariable,value)

+   end

+ end

+ -- All the Go packaging automation relies on goipath being set

+ local goipath = rpm.expand("%{?goipath}")

+ if (goipath == "") then

I am not familiar with the lua but if this forces the goipath to be set, Let's make it optional for now. I don't want the rpmbuild to fail just because the macro is not set.

+   rpm.expand("%{error:Please set the Go import path in the “goipath” variable before calling “gometa”!}")

+ end

+ -- Compute and set spec variables

+ if (forgeurl ~= "") then

+   rpm.expand("%forgemeta %{?-v} %{?-i} %{?-s} %{?-p} -u " .. forgeurl .. "\\n")

+   safeset("gourl", forgeurl)

+ else

+   safeset("gourl", "https://" .. goipath)

+   rpm.expand("%forgemeta %{?-v} %{?-i} -s     %{?-p} -u %{gourl}\\n")

+ end

+ if (rpm.expand("%{?forgesource}") ~= "") then

+   safeset("gosource", "%{forgesource}")

+ else

+   safeset("gosource", "%{gourl}/%{archivename}.%{archiveext}")

+ end

+ safeset("goname", "%gorpmname %{goipath}")

+ -- Final spec variable summary if the macro was called with -i

+ if (rpm.expand("%{?-i}") ~= "") then

+   rpm.expand("%{echo:Go-specific packaging variables}")

+   rpm.expand("%{echo:  goipath:         %{?goipath}}")

+   rpm.expand("%{echo:  goname:          %{?goname}}")

+   rpm.expand("%{echo:  gourl:           %{?gourl}}")

+   rpm.expand("%{echo:  gosource:        %{?gosource}}")

+ end}

+ BuildRequires: compiler(go-compiler)}

+ ExclusiveArch: %{go_arches}

+ }

I'm not sure that converting the package name/import path to lowercase is always desirable. Naming guidelines doesn't forbid having packages with camel case or similar. Not saying that I don't prefer case uniformity, but we should respect(or make possible to respect) upstream naming/import.

Have you considered implementing this command in the compilers meta package(so you can depend on the golang compiler) and using "go list ./..." command and some output formatting, directory traversal?

Oh... I think that this creates implicit dependency on go command. I think that is not a thing that we want to have in minimal build root. Looking on this I think that this script would be better part of the go-compiler package.

Have you considered "go list" for this script. I think that this script would be much simpler and future proof with usage of go list. And due to this better to be part of the go-compilers.

I think this could be replace with %gotest ./..., possibly by appending possibility to pass arg "-run regexp". IMHO in general there shouldn't be need to skip individual test cases, i.e. test case should check for special conditions and fail gracefully, when not met.

I think it is not necessary, if I understand correctly that the dependent change is already part of minimal build root in rawhide.

Could you extend the implementation with some comments? It is not always straightforward to see what is the expected behavior.

This will not cover cases of imported packages of generated Go files. E.g. one needs to run "make gen" in Kubernetes to generate other go files which have their own dependencies.

I am not familiar with the lua but if this forces the goipath to be set, Let's make it optional for now. I don't want the rpmbuild to fail just because the macro is not set.

What about the go.buildreq? Without that non-binary packages will build fine and any missing dependencies will get discovered during runtime. Which will make the maintenance even harder.

I'm not sure that converting the package name/import path to
lowercase is always desirable. Naming guidelines doesn't forbid having
packages with camel case or similar.

That's kind of an historical RHL artifact. Casing package names has caused no end of problems, but by the time the problems were identified it was not possible to force lower-casing without changing a large number of existing packages, mostly perl-side. Other rpm distros did bite that particular bullet since then. Remember that Fedora is an old distribution, with roots in RHL, and early RHL versions were a lot less strict and professional that today's Fedora.

There was a period when it was considered sexy to name his project foo-Kit and the distribution was infected with new camelcased packages. The people who committed those quietly dropped the practice in the next software generation – I guess they got sick of the fallout each time someone mis-cased their software in yum/dnf or another package-oriented tool.

The consensus these days seems to let parts of Fedora mandate lowercasing when they can get it done, while "only" promoting it strongly for the parts of the distro that can not get themselves organized.

From https://fedoraproject.org/wiki/Packaging:Naming?rd=Packaging:NamingGuidelines#General_Naming

“Package names should be in lower case and use dashes in preference of underscores”

This is certainly the right time to do it for Fedora Go packages since making the most of autodeps will require revisiting existing spec files anyway. And non-revisited packages won't use the macro that lowercases, so it does not impact them.

Have you considered implementing this command in the compilers
meta package(so you can depend on the golang compiler)

The go.prov / go.req / go.attr / macro-go-rpm files can certainly be moved to go compilers if you want. At the time they are used go-compilers is available in the build root anyway. It's pulled in by the previous-to-last line in %gometa

Moving to go compilers will be a bit cleaner, at the cost of splitting the macros in multiple source packages. A more natural way to do it would have been to merge go-srpm-macros and go-compilers in a go-macros src.rpm, and make it generate two subpackages: go-srpm-macros (in the default build root) and go-rpm-macros (pulled in only when building Go packages with the trick %gometa uses)

Either way the current implementation does not force golang or go-compilers in the default build root. It's only pulled in when necessary.

and using "go list ./..." command and some output formatting, directory
traversal?

I started by using go list but it didn't work: some projects import project directories, that the origin project does not consider part of itself (because they do not contain go files but assets for example). Plus, invoking go list is a lot (a lot lot lot lot) slower than just running some shellcode.

So in the end making every installed directory a golang() provide via shell processing proved more reliable and more efficient.

Requires have other needs and do use go list.

I think this could be replace with %gotest ./..., possibly by appending
possibility to pass arg "-run regexp". IMHO in general there shouldn't be
need to skip individual test cases, i.e. test case should check for special
conditions and fail gracefully, when not met.

I'm certainly not wed to the current implementation of %gochecks. It is, IMHO, fugly (and I wrote it). Its main redeeming feature is that it works. I don't ever want to get back to listing subdirectories by hand and run %{gotests} on each of them manually, forgetting to add new dirs every time a package is switched to a new version/commit.

If you find a better reliable system to run gotests recursively, while allowing opting-out problem tests, I'll happily switch my specs to it. In the meantime the current implementation is better than nothing.

I don't like the blacklisting much but given the current state of Go code in the wild requiring every package to pass all tests will result in no Go packages in Fedora (even with perfect unit tests opting out is necessary when bootstrapping). At least %gochecks is opt-out, not opt-in

I think it is not necessary, if I understand correctly that the dependent > change is already part of minimal build root in rawhide.

Yes it is and that's the version that added the change to rawhide. However, it's in rawhide only right now. So the check is mostly there to prevent people from trying to rebuild the src.rpm on a stable release, and wondering why nothing works.

You can remove it but I'd rather have it stay till we decide if the change is actually pushed to Fedora stable, or EPEL (and if the change is pushed to all releases or just some of them).

Could you extend the implementation with some comments? It is not
always straightforward to see what is the expected behavior.

Sure, can you provide some guidance of what you'd like to be explained/commented? IIRC it started much simpler but handling symlinks to alias past import paths required some tricky symlink deferencing

This will not cover cases of imported packages of generated Go files.
E.g. one needs to run "make gen" in Kubernetes to generate other go
files which have their own dependencies.

I haven't hit this case so far but remember that go.req computes requirements from the files a package installs in $GOPATH for use by other packages. I'd expect kubernetes to run "make gen" in its own %build stage, and export the result (generated and non-generated files) for other packages. That's what etcd does for example.

Or, are projects expected to run make gen themselves on the kubernetes code they import? In that case I see two solutions:
1. there is some sort of standard way a Go project documents the needs of something like 'make gen' (point me to it and I'll see if it is automate-able. Stuff lost in a general vendoring dump does not count)
2. such things are project-specific, or not automatable, which means the packager of a project will have to add the corresponding Requires manually to his spec.

I am not familiar with the lua but if this forces the goipath to be set,
Let's make it optional for now. I don't want the rpmbuild to fail just
because the macro is not set.

It's not that simple, goipath is used by almost all the macros of the proposal, and by the Go autodep scripts, so sure it would be possible to remove this particular test but then things would fail right and left and I'm not certain anything worthwhile would remain.

Even if one changed every single macro, that uses goipath, to take a parameter instead (which would add to macro complexity and be massively packager-unfriendly) you can't pass parameters to autodep scripts, only variables, so you'd end up with spec files that repeat the import path parameter every time they invoke a go-related macro, and end up putting the import path in a goipath variable anyway, to pass it to the autodep engine.

(of course I may be missing something — please correct me if you know another way)

Do you have a particular functional block in mind that you'd like to use without goipath being set?

What about the go.buildreq? Without that non-binary packages will build fine and any missing
dependencies will get discovered during runtime. Which will make the maintenance even
harder.

I'm not sure I completely understand the question. Please correct me if I answer the wrong question.

  1. BuildRequires can not be automated from within rpm because by the time rpm has access to the project archive content, the build root has been setup, and it is not possible to change its content for security reasons. So all first-level BuildRequires need to be explicitly declared inside the spec file. If you want to automate this step you need to preprocess the project archive in another tool, and use the other tool output to fill in BuildRequires in the spec file. The proposal does not address this part (but this particular difficulty exists with or without the proposal).

  2. The imports of the Go code installed in %{gopath}/src/ wiil be analysed at package creation time to generate corresponding Requires in the created golang-foo-devel packages. You do not need to wait for something else to try to build from those sources and fail because one of the imports is missing. Just try to install the resulting -devel rpm and dnf/yum will tell you if the distribution is missing the imported code (alternatively, write a repoquery script). I'm not even sure if koji will let you push the resulting package. Even if it allows injecting packages with broken dependencies in the repository, that's one of the things Fedora nightly checks verify and complain about.

Of course none of those will tell you if the imported packages, when they exist, are at a code level with a compatible API. That takes running as many unit tests as possible and building as much code as possible. Which the proposal does.

Also given how lax some Go projects are API-wise, trying to rebuild every package, with a dependency provided by a just-updated Go package, wouldn't be luxury. That's not something the proposal does but that's something the proposal makes possible, since autodeps clearly identify all the bits provided or needed by Go packages, and put the result in the repository metadata, where other tools can use it. That's certainly something that should be explored with Fedora QA people once there is a significant volume of unbundled Go packages in Fedora with accurate dependencies.

Lastly making upstreams understand they need to be a little more careful with API breakage would certainly help. That's easier to do when the distro includes lots of Go packages, and those are not stuck in antediluvian versions. Success depends on influence and influence depends on past successes.

So, given all this, what do you want me to do?

I noted:
– splitting part of the PR to put the elements needed at rpm, not srpm stage in go compilers
– comment go.prov for Jan (but I'd really like some clarification first of the parts which are too obscure for him so I don't miss the mark with my comments)

Anything else?

I think that is correct from my side. Separate bits that require implicitly go in to package that explicitly requires go, i.e in to go-compilers

Also I would add to the list support for separate test packages(containing all the test files and data), ideally support for shipping prebuilt shared libraries and possibly bits with race detector enabled and separate debug info packages.

Pull-Request has been closed by nim

2 years ago