[pacman-dev] [PATCH] Provides generation for files (a.k.a. rpm fileattrs for makepkg)

[Eli Schwartz]

On 3/7/20 11:12 PM, Carson Black wrote:
> Whenever I'm working in a relatively fresh Arch environment, my work
> flow looks like this:
> 
> - cmake .. -DFLAGS=blahblahblah
> - Wait a few seconds
> - Package providing KF5whatever not found
> - Try and deduce Arch package name from CMake package name

$ pacman -S pkgfile; pkgfile -u; pkgfile -v KF5whatever.cmake
extra/kwhatever 1.0-1     /usr/lib/cmake/KF5Whatever/KF5Whatev
erConfig.cmake

Alternatively, pacman -Fy; pacman -F provides similar functionality to
pkgfile, these days.

> - dnf install cmake(KF5whatever)
> 
> The latter workflow is a lot more efficient, and is exactly what this
> commit introduces to packages generated by makepkg.
> 
> Every file is iterated over at the end of the build process to generate
> additional provides without the packager needing to manually specify
> them.
> 
> The code is architected in a manner designed to make it trivial to add
> new provider autogenerators.
> 
> So far, there are autogenerated providers for:
> - pkgconfig(package)
> - cmake(package)
> - desktop files
>   * app(desktopfilename)
>   * can-open-mimetype(mimetype)
>   * can-open-file-extension(filetype)
> 
> While these automatically generated provides can be used for packaging,
> this is intended more for interactive usage rather than an attempt to
> change how Arch packaging works.

I consider this rpm functionality to be an anti-feature, there are far
too many ways to depend on the exact same thing and none of it is
opt-in. Also, given there are very simple, intuitive tools like pkgfile
or pacman -F, I don't see why such complexity is needed.

> Signed-off-by: Carson Black <uhhadd at gmail.com>
> ---
>  scripts/makepkg.sh.in | 108 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index ca3e7459..10703a91 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -520,6 +520,111 @@ find_libdepends() {
>  	(( ${#libdepends[@]} )) && printf '%s\n' "${libdepends[@]}"
>  }
>  
> +pkgconfig_fileattr() {
> +	case $1 in
> +		--generate-provides)
> +			case $2 in
> +				*.pc)
> +					directory="`dirname ${2}`"
> +					export PKG_CONFIG_PATH="$DIR:$DIR/../../share/pkgconfig"
> +					pkg-config --print-provides "$2" 2>/dev/null | while read name _ _; do
> +						[ -n "$name" ] || continue
> +						echo "pkgconfig($name)"
> +					done
> +					;;
> +			esac
> +			;;
> +		--generate-depends)
> +			;;
> +	esac
> +}
> +
> +cmake_fileattr() {
> +	case $1 in
> +		--generate-provides)
> +			case $2 in
> +				*.cmake)
> +					base="$(basename $2)"
> +					case $2 in
> +						*Config.cmake)
> +							echo "cmake(${base%Config.cmake})"
> +							;;
> +						*-config.cmake)
> +							echo "cmake(${base%-config.cmake})"
> +							;;
> +					esac
> +					;;
> +			esac
> +			;;
> +		--generate-depends)
> +			;;
> +	esac
> +}
> +
> +desktop_file_attr() {
> +	case $1 in
> +		--generate-provides)
> +			case "$2" in
> +				*.desktop)
> +					grep -q "Type=Application" "$2" || return
> +					grep -q "Exec=" "$2" || return
> +					base="$(basename $2)"
> +					echo "app(${base%.desktop})"
> +
> +					IFS=';'
> +					mimetypes=`grep MimeType= $2 | cut -d '=' -f 2`
> +					for mimetype in $mimetypes; do
> +						echo "can-open-mimetype($mimetype)"
> +
> +						IFS=' '

... did you really just overwrite IFS (twice), then not restore it?

> +						filetypes=`grep -v ^\# /etc/mime.types | grep $mimetype | cut -d ' ' | xargs`
> +						for filetype in $filetypes; do

That is a lot of grep and sed, using a deprecated shell construct (you
should never use ``, always use $() instead) and if you're going to
build a list of multiple things then iterate over it, use a real
datatype like arrays, then iterate over "${filetypes[@]}" -- and mind
your quoting.

This would mean you no longer overwrote IFS -- using arrays means you'd
have to properly split them upfront (mapfile has options to specify
custom delimiters, so that works too).

> +							echo "can-open-file-extension($filetype)"
> +						done
> +					done
> +					;;
> +			esac
> +			;;
> +		--generate-depends)
> +			;;
> +	esac
> +}
> +
> +check_pkg_dir() {
> +	if [[ ! -d $pkgdir ]]; then
> +		error "$(gettext "Missing %s directory.")" "\$pkgdir/"
> +		plain "$(gettext "Aborting...")"
> +		exit $E_MISSING_PKGDIR
> +	fi
> +}

I don't even understand what the purpose of this function is supposed to
be, since $pkgdir is a directory created by makepkg and cannot
(normally) fail to exist.

In the event that a user-created PKGBUILD deletes it, create_package()
performs the exact same check after all user-provided functions are run,
after which it cd's into that directory, and then, from within that
directory, executes the write_pkginfo function which you want to extend.

> +find_fileattrs_provides() {
> +	check_pkg_dir
> +
> +	for file in `find $pkgdir`; do

There is a lot of existing, example code in makepkg which operates on
all files in the pkgdir. It is completely unacceptable to do
word-splitting like this, because filenames can and do contain
whitespace. There are 143 existing packages in the official repositories
that will die on this, including core/linux-firmware and... cmake itself
(in /usr/share/cmake-3.16/Help/generator/).

The common bash programming methodology for iterating over filenames
looks like this:

while IFS= read -rd '' filename; do
    ...
done < <(find "$pkgdir" -type f -print0)


Note also that "$pkgdir" itself must be quoted.

> +		for function in $(declare -F); do

... no. Just no.

Under no circumstances will we be iterating over every shell function
*ever* and checking if they exist. You know exactly which ones exist,
run them manually. This is far, far too leaky as-is.

Another method would be as demonstrated in scripts/libmakepkg/tidy.sh by
registering functions in the arrays tidy_remove and tidy_modify. This is
the pluginized approach to extending something.

> +			case $function in
> +				*_fileattr)
> +					$function --generate-provides $file
> +					;;
> +			esac
> +		done
> +	done
> +}
> +
> +find_fileattrs_depends() {
> +	check_pkg_dir
> +
> +	for file in `find $pkgdir`; do
> +		for function in $(declare -F); do
> +			case $function in
> +				*_fileattr)
> +					$function --generate-depends $file
> +					;;
> +			esac
> +		done
> +	done
> +}

It seems like your proposed code has this being dead functionality?

>  find_libprovides() {
>  	local p libprovides missing
> @@ -613,6 +718,9 @@ write_pkginfo() {
>  	mapfile -t provides < <(find_libprovides)
>  	mapfile -t depends < <(find_libdepends)
>  
> +	mapfile -t provides < <(find_fileattrs_provides)
> +	mapfile -t depends < <(find_fileattrs_depends)

This seems like you are deleting all existing provides and depends, and
then recreating them with only the autogenerated fileattrs ones?

If you look at the existing find_lib* functions, they actually rebuild
the existing provides/depends arrays, taking explicit care to return the
existing ones, but in-place modifying any "libfoo.so" style dependencies
with additional versioning metadata.

Perhaps you meant to append, not overwrite? You could start adding at
the end like this: mapfile -t -O ${#provides[@]} provides

>  	write_kv_pair "license"     "${license[@]}"
>  	write_kv_pair "replaces"    "${replaces[@]}"
>  	write_kv_pair "group"       "${groups[@]}"
> 


-- 
Eli Schwartz
Bug Wrangler and Trusted User

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 1601 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/pacman-dev/attachments/20200308/3790bf24/attachment.sig>