Re: [pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
On 12/28/20 10:15 PM, Allan McRae wrote: On 25/12/20 2:05 am, Emil Velikov wrote: On Thu, 24 Dec 2020 at 01:38, Eli Schwartz wrote: On 12/23/20 5:42 PM, Emil Velikov wrote: All the required public API is annotated with SYMEXPORT, so we can just add the meson notation, to hide all the symbols by default. Thus we no longer spill all the internal API into the global namespace. Thanks for noticing this... it's a regression from autotools, which contained in lib/libalpm/Makefile.am: if ENABLE_VISIBILITY_CC if DARWIN AM_CFLAGS += -fvisibility=hidden else AM_CFLAGS += -fvisibility=internal endif endif I wonder if we had a good reason to use "internal" and if we should continue to do so? IIUC it makes it slightly more optimized at the cost of allowing pointers into private functions (e.g. callbacks) used by other programs to segfault. If the output of size is any indication - there is little-to-no optimisation happening. The former produces the same numbers, while the latter claims that binaries built with "internal" are larger by 8 bytes (always). Fwiw the above snippet is the first time I've seen anyone using "internal", after staring at various projects for 5+ years. Can bring it back, simply not sure it brings much. Thanks Emil $ ls 812336 build-internal/libalpm.so.12.0.1 812328 build-hidden/libalpm.so.12.0.1 337176 build-internal/pacman 337168 build-hidden/pacman $ size 3167083080 592 320380 4e37c build-internal/libalpm.so.12.0.1 3167083080 592 320380 4e37c build-hidden/libalpm.so.12.0.1 15528850405808 166136 288f8 build-internal/pacman 15528850405808 166136 288f8 build-hidden/pacman It turns out, we have this: #define SYMHIDDEN __attribute__((visibility("internal"))) But we never flag any functions with this. That would enable a compiler to optimize for speed and not compatibility, in which case using -fvisibility=internal would make a difference. I'm not sure if we removed any usage of that from the codebase, or if it was never there... Hmm... only ever used on alpm_add_target (then _alpm_add_loadtarget) and removed in commit 7f7da2b5fc01f46d28236384540c7ecfdac16a63 which added the AM_CFLAGS instead and marked a bunch of symbols as SYMEXPORT. But that define used to be for visibility("hidden"), then in commit 920b0d2049deb148efe89bfebda03d172b68c1f5 both the (unused) define and the AM_CFLAGS got switched to internal: "this allows for slightly better optimization". Which is about as vague as our knowledge today. "Supposedly better and probably works fine, but no one put out numbers on it". idk, "hidden" is probably just fine. -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
[pacman-dev] [PATCH] doc: make doxygen build from any directory
In the autotools build, it only built in-tree, from cwd = doc/ and resolving doc/../lib/libalpm In the meson build, this accidentally worked if cwd = pacman/builddir/ and resolved to builddir/../lib/libalpm/ But... this should always have been configured with the actual path to the inputs. So, we will now proceed to do so. Fixes building man3 if your out of tree builddir doesn't happen to be a direct subdirectory of the source root. Signed-off-by: Eli Schwartz --- doc/Doxyfile.in | 4 ++-- doc/meson.build | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/doc/Doxyfile.in b/doc/Doxyfile.in index 776318da7..6744e7659 100644 --- a/doc/Doxyfile.in +++ b/doc/Doxyfile.in @@ -117,8 +117,8 @@ WARN_LOGFILE = #--- # Configuration options related to the input files #--- -INPUT = ../lib/libalpm/alpm.h \ - ../lib/libalpm/alpm_list.h +INPUT = @INPUT_DIRECTORY@/../lib/libalpm/alpm.h \ + @INPUT_DIRECTORY@/../lib/libalpm/alpm_list.h INPUT_ENCODING = UTF-8 FILE_PATTERNS = RECURSIVE = NO diff --git a/doc/meson.build b/doc/meson.build index 570dc7654..4aaac5540 100644 --- a/doc/meson.build +++ b/doc/meson.build @@ -135,6 +135,7 @@ meson.add_install_script(MESON_MAKE_SYMLINK, doxygen = find_program('doxygen', required : get_option('doxygen')) if doxygen.found() and not get_option('doxygen').disabled() doxyconf = configuration_data() + doxyconf.set('INPUT_DIRECTORY', meson.current_source_dir()) doxyconf.set('OUTPUT_DIRECTORY', meson.current_build_dir()) doxyfile = configure_file( input : 'Doxyfile.in', -- 2.29.2
Re: [pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
On 12/23/20 5:42 PM, Emil Velikov wrote: All the required public API is annotated with SYMEXPORT, so we can just add the meson notation, to hide all the symbols by default. Thus we no longer spill all the internal API into the global namespace. Thanks for noticing this... it's a regression from autotools, which contained in lib/libalpm/Makefile.am: if ENABLE_VISIBILITY_CC if DARWIN AM_CFLAGS += -fvisibility=hidden else AM_CFLAGS += -fvisibility=internal endif endif I wonder if we had a good reason to use "internal" and if we should continue to do so? IIUC it makes it slightly more optimized at the cost of allowing pointers into private functions (e.g. callbacks) used by other programs to segfault. -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH 1/3] meson: replace objects with link_whole
On 12/23/20 5:42 PM, Emil Velikov wrote: Instead extracting/listing objects from existing static libraries, simply add those to the link list. Note that link_whole is needed otherwise, unused code will be pruned. This is intentional! link_whole is broken, just like the comment says. It is fixed in newer versions of meson than the current minimum. The fix is that link_whole does precisely what extract_all_objects() does. In other words, link_whole is syntactic sugar for extract_all_objects(). I have intended to switch to link_whole, but *only* once we bump the minimum version of meson to something that guarantees the result is functional. I didn't want to bump the minimum version of meson just for this. -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Add `build/` to gitignore
On 12/10/20 9:12 PM, Colin Woodbury wrote: Right, having `meson` do it automatically would work well. I put forth the patch in the first place because the instructions on the Pacman website (master branch), if followed, place the artifacts in `build/`. Other newcomers to the project like me would hit the same thing, so some automated solution would be nice. Is there anything we can do to move that `meson` proposal forward, other than complaining loudly? ;) https://github.com/mesonbuild/meson/pull/8092 The proposal is getting traction. :D -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Add `build/` to gitignore
On 12/10/20 9:40 PM, Ivy Foster wrote: On 10 Dec 2020, at 6:12 pm -0800, Colin Woodbury wrote: Right, having `meson` do it automatically would work well. I put forth the patch in the first place because the instructions on the Pacman website (master branch), if followed, place the artifacts in `build/`. Other newcomers to the project like me would hit the same thing, so some automated solution would be nice. If I may propose an alternative, this seems like an ideal use for .../pacman/.git/info/exclude. For those who don't know, .git/info/exclude is a secondary gitignore file not under version control. It is specifically for files that shouldn't be ignored at the project level, but that an individual developer or user might want to ignore--such as for build artifacts in non-standardized places, or your personal wrapper scripts for testing whatever you're working on. I do this in $HOME/.config/git/ignore since this pattern is shared by multiple meson-using projects and I can't imagine "builddir/" being actually used for non build artifacts. Still would be nice to autogenerate this on meson's side. I'm discussing it with them... -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Add `build/` to gitignore
On 12/9/20 2:14 PM, Colin Woodbury wrote: Since it's a transient directory for build artifacts, nothing in there will ever been committed. Ignoring it also makes IDEs happier during file search. Signed-off-by: Colin Woodbury --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 01975fd2..638dfe39 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ *~ *.o pacman-*.tar.gz +build/* I use builddir/ (and upstream usually recommends that these days since it's less likely to confuse "meson builddir" as a subcommand, compared to "meson build"). This is kind of the problem with trying to ignore a directory that isn't canonical. Instead, I'd prefer to argue upstream in favor of https://github.com/mesonbuild/meson/issues/6509 -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] Location of the Pacman Home Page source?
On 12/4/20 11:43 AM, Colin Woodbury wrote: Thanks Eli! It looks like I had been grepping for `meson compile` and not `meson build`, hence I couldn't find it. It seems like the asciidoc was already updated some time ago, just not uploaded to the site itself. Should be a simple matter of bumping the version in https://github.com/archlinux/infrastructure/blob/master/playbooks/tasks/pacman-website.yml The 6.0 release will need the playbook to be updated to use meson instead of configure/make. Also, in both `index.asciidoc` and `NEWS`, 5.2.2 was never given release notes. Should we add those, or just wait for the impending 6.0? It got updated, appropriately, on the release/5.2.x branch. That will probably get synced once I write the release notes for 6.0, as happened last time... -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] Location of the Pacman Home Page source?
On 12/4/20 11:20 AM, Colin Woodbury wrote: Hi folks, I see that while a number of Pacman-related pages like the HACKING document are rendered into nice HTML and hosted officially, does anyone know where the source for the Pacman homepage itself is hosted (https://www.archlinux.org/pacman/)? There are some documentation lines I'd like to fix, in particular the ones that still mention configure/make as the way to compile Pacman. Meson should probably also be mentioned in the Pacman README itself, since it is otherwise cited nowhere in that repo. I'd be happy to put forth a PR. In the doc/ directory, which contains asciidoc sources for both mapages and the website. You can generate the html files using `ninja html`, and tar it up for copying onto the website using `ninja doc/website.tar.gz`. -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] [GIT] The official pacman repository tag, last-deltapkgs-commit, created. v5.1.1-144-g377d4714
On 12/3/20 11:01 PM, Allan McRae wrote: On 4/12/20 1:54 pm, Allan McRae wrote: This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "The official pacman repository". The tag, last-deltapkgs-commit has been created at 377d47142f7aaa01ca782e6587f2d4caf663865b (commit) - Log - commit 377d47142f7aaa01ca782e6587f2d4caf663865b Author: Jelle van der Waa Date: Thu Feb 21 13:20:19 2019 +0100 doc: add man page for pacman-conf Signed-off-by: Jelle van der Waa Signed-off-by: Allan McRae --- hooks/post-receive Hrm... I made the v6.0.0alpha1 tag and pushed the tags and this came along for the ride. Not sure where it came from! Anyone know? I think you might have fetched it from https://git.archlinux.org/users/eschwartz/pacman.git/tag/?h=last-deltapkgs-commit But I'm no longer sure why I created this (I think) either. -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH v5] Add fossil scm support to makepkg
exit 1 + esac + fi + + if ! fossil update "$ref"; then Does this need to be silenced too? If I add #tag=0.3.1 to your PKGBUILD... -> Creating working copy of ddate.fossil fossil repo... Autosync: https://iff.ink/fossil/ddate Round-trips: 1 Artifacts sent: 0 received: 0 Pull done, sent: 316 received: 1763 ip: 198.58.102.79 REMOVE .fossil-settings/ignore-glob ADD .gitignore ADD Makefile UPDATE README.md ADD ddata/meta/version ADD ddata/wisdom/yearlength UPDATE doc/ddate.1 REMOVE meson.build --- updated-to: eb8e917743ce4f3d212e67a0f3f09df0eee9b4d1 2020-05-05 05:35:21 UTC tags: trunk, 0.3.1, release comment: Implement actually printing BS dates (user: escond...@iff.ink) changes: 8 files modified. "fossil undo" is available to undo changes to the working checkout. ==> Starting pkgver()... It would be quite nice if fossil had a flag to only summarize but not print info about every single file. >/dev/null is kind of a blunt axe. Then again, idk how important this status is to begin with. + error "$(gettext "Failure while creating working copy of %s %s repo")" "${repo}" "fossil" + plainerr "$(gettext "Aborting...")" + exit 1 + fi + + popd &>/dev/null +} diff --git a/scripts/libmakepkg/source/meson.build b/scripts/libmakepkg/source/meson.build index 59326133..41b18c37 100644 --- a/scripts/libmakepkg/source/meson.build +++ b/scripts/libmakepkg/source/meson.build @@ -3,6 +3,7 @@ libmakepkg_module = 'source' sources = [ 'bzr.sh.in', 'file.sh.in', + 'fossil.sh.in', 'git.sh.in', 'hg.sh.in', 'local.sh.in', diff --git a/scripts/libmakepkg/util/source.sh.in b/scripts/libmakepkg/util/source.sh.in index be7c15c2..029bf8ed 100644 --- a/scripts/libmakepkg/util/source.sh.in +++ b/scripts/libmakepkg/util/source.sh.in @@ -65,7 +65,7 @@ get_filename() { local proto=$(get_protocol "$netfile") case $proto in - bzr|git|hg|svn) + bzr|fossil|git|hg|svn) filename=${netfile%%#*} filename=${filename%%\?*} filename=${filename%/} @@ -73,6 +73,9 @@ get_filename() { if [[ $proto = bzr ]]; then filename=${filename#*lp:} fi + if [[ $proto = fossil ]]; then + filename=$filename.fossil + fi if [[ $proto = git ]]; then filename=${filename%%.git*} fi -- 2.29.2 -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH v2] Add fossil scm support to makepkg
ure $db is quoted (here and elsewhere), there's no reason $SRCDEST / $srcdir cannot contain whitespace. + error "$(gettext "%s is not a clone of %s")" "$db" "$url" + plainerr "$(gettext "Aborting...")" + exit 1 + fi + msg2 "$(gettext "Updating %s %s repo...")" "${repo}" "fossil" + if ! fossil pull -R $db; then + # only warn on failure to allow offline builds + warning "$(gettext "Failure while updating %s %s repo")" "${repo}" "fossil" + fi + fi +} + +extract_fossil() { + local netfile=$1 tagname + + local fragment=$(get_uri_fragment "$netfile") + local repo=$(get_filename "$netfile") + + local db=$(get_filepath "$netfile") + [[ -z "$db" ]] && db="$SRCDEST/$(get_filename "$netfile")" + local dir=${db%%.fossil} + dir=${dir##*/} + + msg2 "$(gettext "Creating working copy of %s %s repo...")" "${repo}" "fossil" + pushd "$srcdir" &>/dev/null + + if [[ -d "$dir" ]]; then + if [[ -f "$dir/.fslckout" ]]; then + cd_safe "$dir" + if ! (fossil info | awk '/^repository:/ {print $2}' | grep "$db" >/dev/null); then + error "$(gettext "Failure while updating working copy of %s %s repo")" "${repo}" "fossil" This seems to be a check for " is not a working copy of ", you've moved up the check from the v1 patch and ensured it correctly checks, but the error message is no longer accurate. Similar issue to above w.r.t. grep -q, () subshell, and [[ + plainerr "$(gettext "Aborting...")" + exit 1 + fi + cd_safe "$srcdir" + else + error "$(gettext "Failure while updating working copy of %s %s repo")" "${repo}" "fossil" + plainerr "$(gettext "Aborting...")" + exit 1 This seems to be correct now... the working copy doesn't strictly need to be updated like in git, but this line here will run whenever the working copy is not, in fact, a fossil repo at all. Which means we won't be able to update it down below since we fail early. lgtm + fi + elif ! fossil open "$db" --workdir "$dir"; then + error "$(gettext "Failure while creating working copy of %s %s repo")" "${repo}" "fossil" + plainerr "$(gettext "Aborting...")" + exit 1 + fi + + cd_safe "${dir##*/}" + + ref=tip + if [[ -n $fragment ]]; then + case ${fragment%%=*} in + branch|commit|tag) + ref=${fragment##*=} + ;; + *) + error "$(gettext "Unrecognized reference: %s")" "${fragment}" + plainerr "$(gettext "Aborting...")" + exit 1 + esac + fi + + if ! fossil update "$ref"; then + error "$(gettext "Failure while creating working copy of %s %s repo")" "${repo}" "fossil" + plainerr "$(gettext "Aborting...")" + exit 1 + fi + + popd &>/dev/null +} diff --git a/scripts/libmakepkg/source/meson.build b/scripts/libmakepkg/source/meson.build index 59326133..41b18c37 100644 --- a/scripts/libmakepkg/source/meson.build +++ b/scripts/libmakepkg/source/meson.build @@ -3,6 +3,7 @@ libmakepkg_module = 'source' sources = [ 'bzr.sh.in', 'file.sh.in', + 'fossil.sh.in', 'git.sh.in', 'hg.sh.in', 'local.sh.in', diff --git a/scripts/libmakepkg/util/source.sh.in b/scripts/libmakepkg/util/source.sh.in index be7c15c2..029bf8ed 100644 --- a/scripts/libmakepkg/util/source.sh.in +++ b/scripts/libmakepkg/util/source.sh.in @@ -65,7 +65,7 @@ get_filename() { local proto=$(get_protocol "$netfile") case $proto in - bzr|git|hg|svn) + bzr|fossil|git|hg|svn) filename=${netfile%%#*} filename=${filename%%\?*} filename=${filename%/} @@ -73,6 +73,9 @@ get_filename() { if [[ $proto = bzr ]]; then filename=${filename#*lp:} fi + if [[ $proto = fossil ]]; then + filename=$filename.fossil + fi if [[ $proto = git ]]; then filename=${filename%%.git*} fi -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Add fossil scm support to makepkg
fragment=$(get_uri_fragment "$netfile") > + local repo=$(get_filename "$netfile") > + > + local db=$(get_filepath "$netfile") > + [[ -z "$db" ]] && db="$SRCDEST/$(get_filename "$netfile")" > + local dir=${db%%.fossil} > + dir=${dir##*/} > + > + msg2 "$(gettext "Creating working copy of %s %s repo...")" "${repo}" > "fossil" > + pushd "$srcdir" &>/dev/null > + > + if [[ -f "$dir/.fslckout" ]]; then > + cd_safe "$dir" > + if ! (fossil revert && fossil clean --verily); then > + error "$(gettext "Failure while updating working copy > of %s %s repo")" "${repo}" "fossil" > + plainerr "$(gettext "Aborting...")" > + exit 1 > + fi This entire block feels terribly wrong. In git, we fetch the $SRCDEST version into the $srcdir one, since they're different git repos wired together as remotes. In fossil, it looks like this is unnecessary and should be skipped (the metadata is directly referenced and opened in $srcdir). Instead, what this is doing is trying to break incremental builds by deleting all untracked files (we do not do this for other sources, but let you use makepkg -C to nuke $srcdir for this purpose), but not modifying the potential revision at all, then declaring this to be the "updating working copy" step of the extraction. > + cd_safe "$srcdir" > + elif [[ -d "$dir" && ! -f "$dir/.fslckout" ]]; then > + error "$(gettext "%s is not a working copy of %s")" "$dir" "$db" > + plainerr "$(gettext "Aborting...")" > + exit 1 This string is new. Shouldn't fossil detect this automatically? How does checking for the existence of the metadata file tell you which repo it is a clone of? > + elif ! fossil open "$db" --workdir "$dir"; then > + error "$(gettext "Failure while creating working copy of %s %s > repo")" "${repo}" "fossil" > + plainerr "$(gettext "Aborting...")" > + exit 1 > + fi > + > + cd_safe "${dir##*/}" > + > + ref=tip > + if [[ -n $fragment ]]; then > + case ${fragment%%=*} in > + branch|checkin|ci|commit|tag) > + ref=${fragment##*=} > + ;; > + *) > + error "$(gettext "Unrecognized reference: %s")" > "${fragment}" > + plainerr "$(gettext "Aborting...")" > + exit 1 > + esac > + fi > + > + if ! fossil update "$ref"; then > + error "$(gettext "Failure while creating working copy of %s %s > repo")" "${repo}" "fossil" "$ref" > + plainerr "$(gettext "Aborting...")" > + exit 1 > + fi > + > + popd &>/dev/null > +} > diff --git a/scripts/libmakepkg/util/source.sh.in > b/scripts/libmakepkg/util/source.sh.in > index be7c15c2..029bf8ed 100644 > --- a/scripts/libmakepkg/util/source.sh.in > +++ b/scripts/libmakepkg/util/source.sh.in > @@ -65,7 +65,7 @@ get_filename() { > local proto=$(get_protocol "$netfile") > > case $proto in > - bzr|git|hg|svn) > + bzr|fossil|git|hg|svn) > filename=${netfile%%#*} > filename=${filename%%\?*} > filename=${filename%/} > @@ -73,6 +73,9 @@ get_filename() { > if [[ $proto = bzr ]]; then > filename=${filename#*lp:} > fi > + if [[ $proto = fossil ]]; then > + filename=$filename.fossil > + fi > if [[ $proto = git ]]; then > filename=${filename%%.git*} > fi > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] pacman-key: change signing key to ed25519
On 11/4/20 5:47 PM, Geert Hendrickx via pacman-dev wrote: > On Wed, Nov 04, 2020 at 16:30:19 -0500, Eli Schwartz wrote: >> Currently pacman assumes gpgme from >= the year 2010, is that sufficient >> to read ed25519? (idk, it's shelling out to gpg and thus likely doesn't >> care?) Maybe we should bump this anyway in the expectation that requiring >> a ~2015 version of gpgme will naturally lead to gpg versions that support >> generating such keys. > > > This change only affects new installations, existing ones will continue > using their rsa2048 (or recently rsa4096) master keys, until they re-run > pacman-key --init. That's really not my point at all. My point is that rerunning --init does something the project dependencies don't describe as a requirement to support. >>> This will also become the default in the next version of GnuPG. >> >> I see such a commit on GnuPG's master branch but not on the stable >> branch. When do you expect this to be released... > > > Good question, I don't know. The point is that the trend is clearly > towards EdDSA rather than larger RSA. And GnuPG (as well as openssh > etc) need to be conservative, as they must be interoperable with other > or older implementations, pacman doesn't even have that limitation. Why doesn't pacman have this limitation? Because it is only used in Arch Linux? Untrue, there is an active MSYS2 community using it, and we support running on macOS/BSD and occasionally get people posting compilation fixes on those platforms. Who knows what version of GnuPG various minor users might have? It's not ridiculous to consider *if* we can declare a dependency on the proposed, updated runtime workflow requirement. Likewise, I think "GnuPG did not actually change this default and won't for some time" is a meaningful point of discussion; Jonas pointed out the key strength actually decreases, while the smaller key size is not obviously beneficial as it is only used for web of trust signing. TBH, I'm -1 on any change that is being done without any rationale other than "because it's the modern way of the future". Changes should be done because they are better, or safer, or some other practical application. This may or may not be the case here, I haven't pondered it much yet. But... it's not being highlighted in the current discussion. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] pacman-key: change signing key to ed25519
On 11/4/20 3:53 PM, Geert Hendrickx via pacman-dev wrote: > Larger RSA keys are not the way forward, switch to ed25519 instead. Currently pacman assumes gpgme from >= the year 2010, is that sufficient to read ed25519? (idk, it's shelling out to gpg and thus likely doesn't care?) Maybe we should bump this anyway in the expectation that requiring a ~2015 version of gpgme will naturally lead to gpg versions that support generating such keys. > This will also become the default in the next version of GnuPG. I see such a commit on GnuPG's master branch but not on the stable branch. When do you expect this to be released... > Signed-off-by: Geert Hendrickx > --- > scripts/pacman-key.sh.in | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in > index ccfd1b96..fd9d3793 100644 > --- a/scripts/pacman-key.sh.in > +++ b/scripts/pacman-key.sh.in > @@ -147,8 +147,8 @@ generate_master_key() { > # Generate the master key, which will be in both pubring and secring > "${GPG_PACMAN[@]}" --gen-key --batch < %echo Generating pacman keyring master key... > -Key-Type: RSA > -Key-Length: 4096 > +Key-Type: EDDSA > +Key-Curve: ed25519 > Key-Usage: sign > Name-Real: Pacman Keyring Master Key > Name-Email: pacman@localhost > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] libmakepkg: lint all arrays for empty values
On 10/27/20 9:17 PM, Morgan Adamiec wrote: > So as it turns out -Qip shows the licenses. Installing the package and > doing -Qi only shows a. Confirmed... In /var/lib/pacman/local/e-1-1/desc I do see: ``` %LICENSE% 1 all the extra licenses even in the middle ``` $ pacman -Qi e Licenses: 1 all the extra licenses This makes a definite case for linting this by accepting your patch. OTOH maybe we should fix this... this /whatever/. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] libmakepkg: lint all arrays for empty values
On 10/27/20 8:59 PM, Morgan Adamiec wrote: > On 28/10/2020 00:46, Eli Schwartz wrote: >> It remains unclear to me, why the ambiguity matters. >> > > I guess if you were to officially declare `Key =` means to set the key > to none then that would solve the issue of it being ambiguous > > Then again that would make `license=('a' '' 'b')` parse as only as > licence=(b) seems as it was cleared before. > > But funnily enough, if you build the package then pacman -Qi reports > only license a. Fun! I just checked this, but -Qip reported all the licenses, both before and after the '' license. Is this an observation about the current state of pacman, or about what the proposed semantics would be? -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] libmakepkg: lint all arrays for empty values
On 10/27/20 8:25 PM, Morgan Adamiec wrote: > On 28/10/2020 00:04, Allan McRae wrote: > >> pkgnames/depends/etc where it may be an issue. So I'm not sure this >> check finds anything in the "break makepkg/pacman" category. > > I disagree, it actually does break something, the srcinfio file > > Consider the following pkgbuild: > > pkgbase=foo > pkgname=(a b) > pkgver=1 > pkgrel=1 > arch=(any) > > license=(1) > > package_a() { > license=() > > } > > package_b() { > license=('') > } > > And the srcinfo file: > > pkgbase = foo > pkgver = 1 > pkgrel = 1 > arch = any > license = 1 > > pkgname = a > license = > > pkgname = b > license = > > > Now package `a` overrides license to an empty array. The srcinfo > expresses this by putting an empty license entry. > > Now package b defines a license of `empty string`, yet it generates the > same output. So it's impossible to tell what the original pkgbuild > actually meant. In both cases, it overrides the license=('1') into non-existence. The question is if "There is no license" or "the license is a zero-length string of emptiness" is semantically meaningful. Especially given the author of the PKGBUILD probably intended to put the former. Given it's a purely display-oriented field, you can absolutely stuff it with whatever you want, and "a zero-length string of emptiness" is not technically invalid even if it is stupid, whereas it's useful to catch makedepends=('') before makepkg -s fails to find a satisfier, root and all. > This example may seen a little contrived, but i assure you it's not. > Because stuff like this is done in the wild [1] and as a maintainer of a > srcinfo parser it's annoying that it creates this ambiguity. It remains unclear to me, why the ambiguity matters. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH 1/2] makepkg: fix signing of source packages
On 10/11/20 10:22 PM, Eli Schwartz wrote: > In commit c6b04c04653ba9933fe978829148312e412a9ea7 the signing stage was > moved out of fakeroot, and thus into the main control flow instead of > create_{,src}package Random factoid: 10:47 PM eschwartz: had I seen those patches before? 10:48 PM no, I wrote them tonight 10:49 PM ok - it seemed familiar 10:49 PM you previously merged: 10:49 PM makepkg: when signing packages, report package filename on failure 10:49 PM libmakepkg/integrity: fix regression that broke --install 10:49 PM makepkg: avoid false "Signing package(s)" msg when signing is disabled 10:50 PM good grief 10:50 PM great patch :P 10:50 PM that commit is the gift that keeps on giving I must say, this patch really was fun. :D How on earth did it manage to have so many edge cases Bets on us discovering a 5th problem? -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH 1/2] makepkg: fix signing of source packages
In commit c6b04c04653ba9933fe978829148312e412a9ea7 the signing stage was moved out of fakeroot, and thus into the main control flow instead of create_{,src}package While the function for signing binary packages has logic to build and gpg-sign multiple filenames, the source package never got this special treatment. This would be fine, except it uses the standard variables to set define the filename... like ${fullver}, which is usually set beforehand, but in this case is not. We don't define fullver globally as it's an internal implementation detail, except by sheer coincidence if PKGVERFUNC is false due to improperly guarded code. Result: source packages didn't end up signed. Instead, we raised a logic error: ==> WARNING: Failed to sign package file somepackage-.src.tar.gz. ==> ERROR: An unknown error has occurred. Exiting... Instead, let's just build the version inline, since we only use it once. Reported-by: GaKu999 Signed-off-by: Eli Schwartz --- scripts/makepkg.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index e1e95412..a9e7c691 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1362,7 +1362,7 @@ if (( SOURCEONLY )); then if [[ $SIGNPKG = 'y' ]]; then msg "$(gettext "Signing package...")" - create_signature "$SRCPKGDEST/${pkgbase}-${fullver}${SRCEXT}" + create_signature "$SRCPKGDEST/${pkgbase}-$(get_full_version)${SRCEXT}" fi msg "$(gettext "Source package created: %s")" "$pkgbase ($(date +%c))" -- 2.28.0
[pacman-dev] [PATCH 2/2] makepkg: properly localize some internal function variables
We leaked fullver and pkgarch all over the place, and only conditionally unset the other variables. Marking them local is a more proactive solution. Signed-off-by: Eli Schwartz --- scripts/makepkg.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index a9e7c691..89da3fab 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -869,6 +869,7 @@ install_package() { } check_build_status() { + local fullver pkgarch allpkgbuilt somepkgbuilt if (( ! SPLITPKG )); then fullver=$(get_full_version) pkgarch=$(get_pkg_arch) @@ -911,7 +912,6 @@ check_build_status() { exit $E_ALREADY_BUILT fi fi - unset allpkgbuilt somepkgbuilt fi } -- 2.28.0
Re: [pacman-dev] [PATCH] makepkg: libprovides: don't provide both versioned and unversioned sonames
On 6/26/20 2:09 AM, Allan McRae wrote: > This patch is fine, but I don't like "newp" as a variable name. It will > not help me understand this in a years time. > > How about versioned_provides=() instead? Yes, that sounds fine to me. Do you want to do a simple replacement while pulling the patch, or should I resubmit it? -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] Replace Pacman blurry icon with vector
On 9/19/20 9:51 AM, Morten Linderud wrote: > On Wed, Sep 16, 2020 at 04:34:58PM -0400, Eli Schwartz wrote: >> pacman doesn't come with any icons. What program are you actually using? > > It's pamac, so no relevance to pacman and complete unrelated. > > https://gitlab.manjaro.org/applications/pamac/-/issues/635 I got an off-list email from the OP a day earlier, fingering the same third-party program. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] repo-add: add --include-sigs option
On 9/21/20 3:02 AM, Allan McRae wrote: > On 21/9/20 3:51 pm, Andrew Gregory wrote: >> I would suggest just allowing the user to specify either way >> (--include-sigs/--no-include-sigs, --include-sigs={yes,no}, etc). >> Then uses can specify whatever they want without having to worry about >> what we set as a default. >> > > The problem is more the transition. I would like the default to be not > to include the signatures in the repo database. So does pacman need to > manage the transition from having signatures in a database to not, or do > the users need to manage that? > > With my patch (or any variant the does not include signatures by > default), users upgrading to repo-add v6.0 would need to adjust their > repo management utilities to add a signature include option immediately, > as their users may still be using pacman-5.x. > > Thinking of Arch here, a dbscripts update would need launched on the > server at the same time as updating repo-add. I am OK with that - some > updates need done in concert. But Eli was not. I'm concerned both about the need to time the adjustment of the option, and about the desire for what I see as sane defaults. My preference is to provide both options, but change the default in pacman 6.0.1. While we're hacking on repo-add options, we could go ahead and make it use parseopts, because the current handling is gross. Also I would like an elephant (usually I would request a pony, but this felt more apropos if we're talking about repo-add). -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] Replace Pacman blurry icon with vector
On 9/16/20 10:23 AM, Lucas Bustamante wrote: > Hi, > > First of all, thanks for contributing to this open-source program. > > Can you please update Pacman icon to vector? It's a bit blurry :) > > [image: image.png] pacman doesn't come with any icons. What program are you actually using? -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH 0/1] Introduce "AssumeInstalled" option in the config file
On 9/5/20 3:11 AM, Damjan Georgievski wrote: > On Fri, 4 Sep 2020 at 03:35, Andrew Gregory > wrote: >> Without a better use case, I'm -1 on this. --assume-installed was >> intended for ignoring individual dependencies in exceptional >> circumstances, not permanently ignore a dependency you don't like. >> The preferred way of dealing with that, should you feel the need, is >> with a meta package. > > and how do I plug that into pacstrap? custom repo? that seems like a > much bigger overkill :/ Do you need to install it during pacstrap itself to override packages in the base install? If not, you can simply execute pacstrap && pacman -r /mnt -U /path/to/metapackage. About a year ago, I added a -U option to pacstrap, which makes it use pacman -U for all packages; you may therefore pipe a list of packages to install as such: mkdir ./fakedb { pacman --dbpath ./fakedb -Syp base; echo /path/to/metapackage; } | pacstrap -U /mnt - Since pacstrap may be repeatedly run on the same existing root and does not require that it be the one to create said root, you may also do this much simpler form (assuming the metapackage does not have dependencies, which it presumably shouldn't): pacstrap -U /mnt /path/to/metapackage pacstrap /mnt [base otherpackages] Either way, I'm even less convinced of the need for a pacman.conf option if the only time it is needed is during pacstrap and never again; even if it were true that you needed it then, I'd argue you could just as well install as normal, then replace the package you didn't want with a metapackage after chrooting in. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] makepkg.conf: Reword "Defaults"
On 9/2/20 10:47 PM, Allan McRae wrote: > FS#61661 notes that we have a comment "Defaults" value for BUILDENV and > OPTIONS > but that does not necessarily correspond to what the example makepkg.conf > sets. > > Change the comment to "Makepkg defaults" to indicate this is what makepkg will > do unless told otherwise. ACK. This seems like a very reasonable clarification to make and I really don't want to back out 44f3a157983e903f926b4f11ddb3f57d111e60f9 :D -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] repo-add: add --include-sigs option
On 9/2/20 11:02 PM, Allan McRae wrote: > Pacman now downloads the signature files for all packages when present in a > repository. That makes distributing signatures within repository databases > redundant and costly. > > Do not distribute the package signature files within the repo databases by > default and add an --include-sigs to revert to the old behaviour. As I've mentioned on the list before, I would like an --ignore-sigs option and continue to distribute sigs by default for pacman 6.0 In pacman 6.1 we'll switch by default to ignoring them, and let people use --include-sigs to revert to the old behavior. Ignoring sigs right out of the gate means the default behavior of repo-add is to be unusable for people upgrading from pacman N-1. For example, Arch Linux would most certainly need to use the option to provide backwards compat while upgrading. So do third-party repositories. Also: this option cannot be added to scripts ahead of time, since repo-add will error on an unknown option, and it cannot be added after the fact, since some packages will be broken in the meantime. I don't see what the rush is here to add behavior that no one will want to use. - It makes sense to make this configurable now that it's useful to be able to ignore them. - At the same time, defaults should be based on what is more likely for people to want. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Add pacman-conf zsh completions
On 9/3/20 9:38 PM, Andrew Gregory wrote: > On 08/21/20 at 08:20pm, Ronan Pigott wrote: >> From: Ronan Pigott >> >> Signed-off-by: Ronan Pigott > > Sure, I guess. pacman-conf is meant for use in scripts; who on Earth > is running it in a terminal? It can be pretty useful for running without options in order to copy-paste your pacman.conf for help, but that's again not really a use case for completions. ... I guess we want the same completions in bash_completion.in, but given I don't really have a good use for it myself, I'm not sure I'll bother writing it myself... -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] NO_MSGSIGNAL and SIGPOLL don't exist on darwin
On 8/31/20 10:52 PM, Cameron Katri wrote: > On darwin NO_MSGSIGNAL and SIGPOLL, don't exist, so SO_NOSIGPIPE will be used > instead, which serves the same function. IIUC this isn't usable in send(), but only in setsockopt(), so I think you'll need a more comprehensive solution to ensure it does more than compile. Also why are you hardcoding the value rather than using the existing SO_NOSIGPIPE define wherever it is found? > From a61c9634831e5aff6cd020148516337dd4f8090f Mon Sep 17 00:00:00 2001 > From: Cameron Katri > Date: Mon, 31 Aug 2020 09:50:09 -0400 > Subject: [PATCH] Fix compilation on Darwin > > --- > lib/libalpm/util.c | 10 +- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c > index b70a8192..f7d40b78 100644 > --- a/lib/libalpm/util.c > +++ b/lib/libalpm/util.c > @@ -34,6 +34,11 @@ > #include > #include > > +/* no MSG_NOSIGNAL support */ > +#ifndef MSG_NOSIGNAL > +#define MSG_NOSIGNAL 0x1022 > +#endif > + > /* libarchive */ > #include > #include > @@ -557,8 +562,11 @@ static void _alpm_reset_signals(void) > int *i, signals[] = { > SIGABRT, SIGALRM, SIGBUS, SIGCHLD, SIGCONT, SIGFPE, SIGHUP, > SIGILL, > SIGINT, SIGKILL, SIGPIPE, SIGQUIT, SIGSEGV, SIGSTOP, SIGTERM, > SIGTSTP, > - SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, SIGPOLL, SIGPROF, SIGSYS, > SIGTRAP, > + SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, SIGPROF, SIGSYS, SIGTRAP, > SIGURG, SIGVTALRM, SIGXCPU, SIGXFSZ, > +#ifndef __APPLE__ > + SIGPOLL, > +#endif > 0 > }; > struct sigaction def; > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Disable embedded signatures by default
On 8/26/20 8:26 PM, Anatol Pomozov wrote: > The purpose of the change is to start using the detached signatures > codepath. The detached signatures are shipped with repos for a long > time and pacman can handle it. Now it is time to actually enable it by > default. > > "UseEmbeddedSignatures" option has been added as a fallback plan in > case we find that the detached signatures codepath is broken. Do you > think this is too much hassle and we should just start using detached > signatures by default without any fallback config option? I already stated my opinion. Firstly, that we should NOT add a configuration option for this, since it is a burden on the manpage and is completely useless except for development testing. Secondly, I believe we should continue to check both, because I see no compelling reason to reject perfectly working functionality for no reason, plus you have no way of knowing if there are thirdparty repositories which locally generate databases with sigs, but then only upload the packages and databases, but not the sigs (on the grounds that they won't be used so why bother). We don't just care about Arch Linux. Also, we don't just care about the official repos, even for Arch Linux use. Before instituting a breaking change, we need a better reason than "this is a convenient way for a pacman developer to test whether or not pacman is broken". >> This is the right approach, yeah. I was thinking we'd wait until pacman >> 6.1 before stopping the signature embedding, to provide a transition >> period for people depending on SigLevel = Required (which should be >> everyone, and certainly includes Arch!) to upgrade to 6.x before >> repo-add starts generating databases useless to pacman 5.x > > There are 2 sets of changes that need to be done: > 1) make pacman to use detached signatures instead of embedded ones > 2) change "repo-add" to avoid adding embedded signatures > > We should release changes for #1 first, test it, make sure that > detached signatures fully work (while dbs still have pacman > 5.x-compatible embedded sigs). And only then release #2 to get smaller > databases compatible with pacman version >= 6.0. > > I was thinking #1 can be released with 6.0 and #2 with 6.1. My vote is to not do #1 at all. I do not see why you keep insisting it "needs" to be done. #2 is all we need to generate test repositories. Here is a script to generate test repositories: ssh pkgbuild.com cd public_html/repo mkdir x86_64-detachedsigs cd mkdir x86_64-detachedsigs bsdtar -xOf ../x86_64/eschwartz.db.tar.gz | awk '/^%FILENAME%/{getline;print}' | while read -r line; do cp -v ../x86_64/"$line" ./ done repo-add eschwartz.db.tar.gz *.pkg.tar* bsdtar -xOf ../x86_64/eschwartz.db.tar.gz | awk '/^%FILENAME%/{getline;print}' | while read -r line; do cp -v ../x86_64/"$line".sig ./ done Here is a test repo you can verify against: [eschwartz] Server = https://pkgbuild.com/~eschwartz/repo/x86_64-detachedsigs/ -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH 0/1] Introduce "AssumeInstalled" option in the config file
On 8/30/20 1:18 PM, Damjan Georgievski wrote: > On Sun, 30 Aug 2020 at 19:13, Eli Schwartz wrote: >> aside: for perl, see https://bugs.archlinux.org/task/54887 > > I know that issue (hint: see the reporter) > :) haha :p I did not notice that. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH 0/1] Introduce "AssumeInstalled" option in the config file
On 8/30/20 1:07 PM, Дамјан Георгиевски wrote: > I have had at least two or three use cases for this option, > * one has been since I always have to type > pacman -Syu --assume-installed noto-fonts when plasma-integration is updated > * `pacman -Syu --assume-installed perl` since it's needlesly pulled by openssl > and I didn't need it for containers and similar > > Having this in the config file allows to use the option together with > pacstrap too. > > Please review and if the change is acceptable suggest what else needs to be > added I'm not sure it's a good idea to encourage this in the general case. Supporting it only as a command-line option is still useful since it can help resolve certain types of manual itervention. One alternative would be to create a dummy package providing it. That being said, I'm not necessarily saying this must be rejected... aside: for perl, see https://bugs.archlinux.org/task/54887 > Дамјан Георгиевски (1): > AssumeInstalled option in the config file > > etc/pacman.conf.in | 1 + > src/pacman/conf.c | 2 ++ > 2 files changed, 3 insertions(+) > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH v4] libalpm: Add support for trigger dropins
On 8/24/20 4:21 PM, Andrew Gregory wrote: > I'm still a bit skeptical. I can't say I'm particularly familiar with > fonts and their configuration, but is there really a valid use case > for that kind of per-package configuration that we need to support? I kind of agree, because while it's true font conf files can add new search paths it's also not unreasonable to just use one unified path, as fontconfig already recursively searches there and you can organize fonts by subdirectory. Nevertheless, thanks Allan for mentioning it, because I really wanted to know if this could be suitably generic for non-kernel use and this gives a much better framework for discussion... > More abstractly, I'm not fond of how much this ties packages together. > One of the ideas of hooks was for the package that does the work to > handle all of the necessary details so the triggering packages > wouldn't have to worry about it. With this approach, they not only > have to know to adjust the hook, they also have to know the name of > the hook and how the hook works. Changes to the hook then require an > adjustment and rebuild of any package modifying it. This is not an > entirely new problem, user overrides are also dependant on hook > naming, but this does exacerbate it in a way that I think will be easy > for packagers to miss and cause breakage. > > What about adding Include support to hooks? Then hooks that need this > type of functionality could explicitly include trigger files from > a particular directory, insulating the process from simple hook > renaming and hopefully making it more obvious when changes to the hook > will require changes to any package that modifies it. I think you're right and Include support would be a better way to go here. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH v4] libalpm: Add support for trigger dropins
ld system, not the Arch packager -- so this would force the packagers to maintain an absolute filelist by hand. Maybe it would be interesting to look into having NoExtract'ed files not trigger hooks, instead. Users could prevent installation of dracut files they don't want to use, and as a bonus, if you suddenly decide to use it you track this by starting to install the file, rather than just building the initrd and then forgetting the hook doesn't watch for rebuilds anymore and having it not rebuild when it should. > Adding more triggers is mostly useful for hook based packages > depending on other hook based packages (kernel-install and > mkinitcpio/dracut is a single example of this) where the nested hook > based package can add its own hook directory as a trigger to the main > package pacman hook. Assuming a package stores some stuff in a > non-standard path and needs another package to run on that > non-standard place when it's updated, it can add a drop-in to add that > path to the list of triggers for that package's pacman hook. You don't need to re-explain to me "what" it does, this is evident in the initial patch submission. :) But again -- I don't see how this is useful for anything other than one arguably wrongly-implemented kernel package (which is not actually implemented yet). My argument is you "shouldn't" do this. ... I'm also not sure what "nested" hooks have to do with this, since pacman hooks aren't nested and kernel-install would only be one hook that (re)builds an initramfs as part of its internal functionality. Do you want to be able to have a hook that is also triggered when another named hook is run? That's a different proposal, but color me intrigued. > I realize that these are mostly niceties and I'm not solving some big > massive issue packaging issue by adding this. Still, since there's not > that much added complexity in the implementation and the user facing > part (imo) so adding this seems worth it to me to make some of the > mentioned use cases a little easier to handle. I'm definitely up for improving niceties, there's no problem there. :) My only concern here is that I don't think this is a nicety -- I think it's an incorrect approach. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH v4] libalpm: Add support for trigger dropins
On 8/22/20 9:12 AM, Daan De Meyer wrote: > In some scenarios, instead of adding their own hooks, packages want to > augment hooks already installed by other packages with extra triggers. > Currently, this requires modifying the existing hook file which is error > prone and hard to manage for package managers. > > A concrete example where packages would want to extend an existing hook > is using systemd's kernel-install script on Arch Linux. An example > pacman hook for kernel-install could be the following: > > ``` > [Trigger] > Operation = Install > Operation = Upgrade > Type = Path > Target = usr/lib/modules/*/vmlinuz > Target = usr/lib/kernel/install.d/* > > [Trigger] > Operation = Install > Operation = Upgrade > Type = Package > Target = systemd > > [Action] > Description = Adding kernel and initramfs images to /boot... > When = PostTransaction > Exec = /etc/pacman.d/scripts/mkosi-kernel-add > NeedsTargets > ``` > > This hook would run on installation and upgrades of kernel packages and > every package that installs kernel-install scripts (which includes > dracut and mkinitcpio). It's also fairly generic in that it doesn't > include specifics of other packages except its source package (and > implicitly the install directory of kernel packages). > > However, both mkinitcpio and dracut use the concept of hooks that > can be installed by other packages to extend their functionality. In > mkinitcpio these are stored in /usr/lib/initcpio. When a package is > installed that installs extra hooks for mkinitcpio, the kernel-install > hook will not trigger. We can fix this by adding /usr/lib/initcpio/* to > the kernel-install pacman hook but this requires either modifying the > kernel-install hook when installing mkinitpcio or adding all possible > directories for all possible packages that want to add triggers to the > kernel-install hook in the systemd package itself. Neither of these are > attractive solutions. The first one isn't because we'd have to modify > the hook when installing packages, the second isn't because it would be > a huge maintenance burden and doesn't include for AUR or private > repository packages. This all seems fairly specific to the kernel itself. As a matter of curiosity... why can't a mkinitcpio hook declare it is triggered on /usr/lib/initcpio and a dracut hook declare it is triggered on /usr/lib/dracut/modules.d/ ? In fact, the current mkinitcpio hook does precisely this, and is provided by the mkinitcpio package. It's all statically coded ahead of time, AFAICT the only reason to do it this way is to incorrectly put a kernel-install hook into the systemd package and make the mkinitcpio/dracut packages dynamically load themselves into said hook. There is currently no kernel-install pacman hook, and even if one did want to declare kernel-install the official way to manage kernels on Arch Linux, the systemd package is the wrong place to put it as systemd is installed on many Arch systems that don't have a kernel and should not run kernel-install hooks. If two packages provide mutually exclusive services, I think it's reasonable for them to also provide mutually exclusive pacman hooks geared to that package specifically. It's not like you can even use kernel-install with both dracut and mkinitcpio installed, since they both try to create the same initramfs. More likely, since kernel-install is only catering to a fairly narrow use case while being installed as part of the base system due to it being bundled as part of systemd itself, it will be the responsibility of people who actually like it to manually (no package) install their own hook to /etc/pacman.d statically coding the desired directory to watch, which will never ever ever change after the initial installation process. It's entirely reasonable for people to create their own custom hooks for their own system, augmenting the packaged hooks which are only supposed to be used for cases where it's unambiguously correct to run those routines. That is why there is a hookdir in /etc. I have a bunch of hooks which are totally unfit for general packaging but I wouldn't wish to live without. Can I hear an argument for where this functionality is generically useful, and in a case which isn't simply incorrect packaging? -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Disable embedded signatures by default
On 8/11/20 9:24 AM, Allan McRae wrote: > On 11/8/20 7:44 am, Eli Schwartz wrote: >> On 8/10/20 5:34 PM, Anatol Pomozov wrote: >>> Switching from embedded to detached signatures is a big change. This >>> feature needs to be thoroughly tested before embedded signatures will be >>> completely removed from the database. >>> >>> To help with detached signatures testing we enable it by default. But in >>> case if an user needs to go back to embedded signatures we add a config >>> option to reenable it - "UseEmbeddedSignatures". >> What is the purpose of this? Either signature source should be >> equivalent, and you should be able to trivially test this by creating a >> repo with unsigned packages, then bulk-signing the packages after they >> were repo-added. I don't believe that pacman should include such an >> end-user option purely to double-check whether a current feature >> actually works. > > Agreed - the user should not care where the signatures come from, so > this option should not exist. > > Also, I see this was proposed on arch-dev-public first. I am not > subscribed there, and decisions on what is included in pacman are not > dictated by Arch Linux. Proposals should be posted here. More specifically -- decisions on what is included in pacman are not dictated by consensus of the Arch Linux team, but by the pacman team (which is in turn guided, but not dictated, by what is useful for archlinux). Making a bad or confusing package manager simply because archlinux wants it, would be a bad move due to making a bad or confusing package manager. > Now, thinking out loud here... Would an alternative be to add an > "--embed-signatures" option to repo-add? So two versions of a repo > could be created and those that want to test without embedded signatures > can. This is the right approach, yeah. I was thinking we'd wait until pacman 6.1 before stopping the signature embedding, to provide a transition period for people depending on SigLevel = Required (which should be everyone, and certainly includes Arch!) to upgrade to 6.x before repo-add starts generating databases useless to pacman 5.x But I'd also be fine with --no-embed-signatures for opting in early, and switching to --embed-signatures for opting out once we default to --no-* -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH] makepkg: --source should download repos with PGP signatures
We optimize this out for sourceballs since VCS sources don't get their checksums verified. But this logic is broken ever since we implemented PGP signature checking for git sources -- if the git source is signed, we still check it, but we don't make sure to download it first. makepkg then fails to generate a sourceball unless you previously ran --verifysource or attempted to build. Signed-off-by: Eli Schwartz --- scripts/libmakepkg/source.sh.in | 5 - scripts/libmakepkg/source/git.sh.in | 9 ++--- scripts/makepkg.sh.in | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/scripts/libmakepkg/source.sh.in b/scripts/libmakepkg/source.sh.in index a0c6b662..b95e6be8 100644 --- a/scripts/libmakepkg/source.sh.in +++ b/scripts/libmakepkg/source.sh.in @@ -35,7 +35,7 @@ done download_sources() { local netfile all_sources - local get_source_fn=get_all_sources_for_arch get_vcs=1 + local get_source_fn=get_all_sources_for_arch get_vcs=1 get_pgp=0 msg "$(gettext "Retrieving sources...")" @@ -47,6 +47,9 @@ download_sources() { novcs) get_vcs=0 ;; + getpgp) + (( SKIPPGPCHECK )) || get_pgp=1 + ;; *) break ;; diff --git a/scripts/libmakepkg/source/git.sh.in b/scripts/libmakepkg/source/git.sh.in index 7d191b8d..d090f14e 100644 --- a/scripts/libmakepkg/source/git.sh.in +++ b/scripts/libmakepkg/source/git.sh.in @@ -29,13 +29,16 @@ source "$LIBRARY/util/pkgbuild.sh" download_git() { + local netfile=$1 + local query=$(get_uri_query "$netfile") + # abort early if parent says not to fetch if declare -p get_vcs > /dev/null 2>&1; then - (( get_vcs )) || return + if (( ! get_pgp )) || [[ $query != signed ]]; then + (( get_vcs )) || return + fi fi - local netfile=$1 - local dir=$(get_filepath "$netfile") [[ -z "$dir" ]] && dir="$SRCDEST/$(get_filename "$netfile")" diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 7e8d6805..c9940f0a 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1416,7 +1416,7 @@ if (( SOURCEONLY )); then download_sources allarch elif ( (( ! SKIPCHECKSUMS )) || \ ( (( ! SKIPPGPCHECK )) && source_has_signatures ) ); then - download_sources allarch novcs + download_sources allarch novcs getpgp fi check_source_integrity all cd_safe "$startdir" -- 2.28.0
[pacman-dev] [PATCH] remove more autotools files
We forgot to remove m4/ in commit 454ea024383eab60295e4c4fdf2c329475887b2c and now it's tragically reminding me of autotools! Also take this opportunity to drop some symlinks in lib/libalpm/ for libcommon source files. In autotools these were built specifically for libalpm and needed to be available in that directory, but the meson setup just has libalpm depend on libcommon. So these pseudo source files aren't needed anymore. Signed-off-by: Eli Schwartz --- lib/libalpm/ini.c | 1 - lib/libalpm/ini.h | 1 - lib/libalpm/util-common.c | 1 - lib/libalpm/util-common.h | 1 - m4/.gitignore | 3 - m4/acinclude.m4 | 179 m4/gpgme.m4 | 238 -- 7 files changed, 424 deletions(-) delete mode 12 lib/libalpm/ini.c delete mode 12 lib/libalpm/ini.h delete mode 12 lib/libalpm/util-common.c delete mode 12 lib/libalpm/util-common.h delete mode 100644 m4/.gitignore delete mode 100644 m4/acinclude.m4 delete mode 100644 m4/gpgme.m4 diff --git a/lib/libalpm/ini.c b/lib/libalpm/ini.c deleted file mode 12 index b16d2f27.. --- a/lib/libalpm/ini.c +++ /dev/null @@ -1 +0,0 @@ -../../src/common/ini.c \ No newline at end of file diff --git a/lib/libalpm/ini.h b/lib/libalpm/ini.h deleted file mode 12 index e395fa04.. --- a/lib/libalpm/ini.h +++ /dev/null @@ -1 +0,0 @@ -../../src/common/ini.h \ No newline at end of file diff --git a/lib/libalpm/util-common.c b/lib/libalpm/util-common.c deleted file mode 12 index cf965176.. --- a/lib/libalpm/util-common.c +++ /dev/null @@ -1 +0,0 @@ -../../src/common/util-common.c \ No newline at end of file diff --git a/lib/libalpm/util-common.h b/lib/libalpm/util-common.h deleted file mode 12 index 988c2acc.. --- a/lib/libalpm/util-common.h +++ /dev/null @@ -1 +0,0 @@ -../../src/common/util-common.h \ No newline at end of file diff --git a/m4/.gitignore b/m4/.gitignore deleted file mode 100644 index 4bc6b527.. --- a/m4/.gitignore +++ /dev/null @@ -1,3 +0,0 @@ -*.m4 -!acinclude.m4 -!gpgme.m4 diff --git a/m4/acinclude.m4 b/m4/acinclude.m4 deleted file mode 100644 index 845c8286.. --- a/m4/acinclude.m4 +++ /dev/null @@ -1,179 +0,0 @@ -dnl acinclude.m4 - configure macros used by pacman and libalpm -dnl Add some custom macros for pacman and libalpm - -dnl GCC_STACK_PROTECT_LIB -dnl adds -lssp to LIBS if it is available -dnl ssp is usually provided as part of libc, but was previously a separate lib -dnl It does not hurt to add -lssp even if libc provides SSP - in that case -dnl libssp will simply be ignored. -AC_DEFUN([GCC_STACK_PROTECT_LIB],[ - AC_CACHE_CHECK([whether libssp exists], ssp_cv_lib, -[ssp_old_libs="$LIBS" - LIBS="$LIBS -lssp" - AC_TRY_LINK(,, ssp_cv_lib=yes, ssp_cv_lib=no) - LIBS="$ssp_old_libs" -]) - if test $ssp_cv_lib = yes; then -LIBS="$LIBS -lssp" - fi -]) - -dnl GCC_STACK_PROTECT_CC -dnl checks -fstack-protector-all with the C compiler, if it exists then updates -dnl CFLAGS and defines ENABLE_SSP_CC -AC_DEFUN([GCC_STACK_PROTECT_CC],[ - AC_LANG_ASSERT(C) - if test "X$CC" != "X"; then -AC_CACHE_CHECK([whether ${CC} accepts -fstack-protector-all], - ssp_cv_cc, - [ssp_old_cflags="$CFLAGS" - CFLAGS="$CFLAGS -fstack-protector-all" - AC_TRY_COMPILE(,, ssp_cv_cc=yes, ssp_cv_cc=no) - CFLAGS="$ssp_old_cflags" - ]) -if test $ssp_cv_cc = yes; then - CFLAGS="$CFLAGS -fstack-protector-all" - AC_DEFINE([ENABLE_SSP_CC], 1, [Define if SSP C support is enabled.]) -fi - fi -]) - -dnl GCC_STACK_CLASH_PROTECTION -dnl check -fstack-clash-protection with the C compiler, if it exists then -dnl updates CFLAGS -AC_DEFUN([GCC_STACK_CLASH_PROTECTION],[ - AC_LANG_ASSERT(C) - if test "X$CC" != "X"; then -AC_CACHE_CHECK([whether ${CC} accepts -fstack-clash-protection], - scp_cv_cc, - [scp_old_cflags="$CFLAGS" - CFLAGS="$CFLAGS -fstack-clash-protection" - AC_TRY_COMPILE(,, scp_cv_cc=yes, scp_cv_cc=no) - CFLAGS="$scp_old_cflags" - ]) -if test $scp_cv_cc = yes; then - CFLAGS="$CFLAGS -fstack-clash-protection" -fi - fi -]) - -dnl GCC_FORTIFY_SOURCE_CC -dnl checks -D_FORTIFY_SOURCE with the C compiler, if it exists then updates -dnl CPPFLAGS -AC_DEFUN([GCC_FORTIFY_SOURCE_CC],[ - AC_LANG_ASSERT(C) - if test "X$CC" != "X"; then -AC_MSG_CHECKING(for FORTIFY_SOURCE support) -fs_old_cppflags="$CPPFLAGS" -fs_old_cflags="$CFLAGS" -CPPFLAGS="$CPPFLAGS -D_FORTIFY_SOURCE=2" -CFLAGS="$CFLAGS -Werror" -AC_TRY_COMPILE([#include ], [ - int main() { - #if !(__GNUC_PREREQ (4, 1) ) - #error No FORTIFY_SOURCE support - #endif
Re: [pacman-dev] [PATCH] Disable embedded signatures by default
On 8/10/20 5:34 PM, Anatol Pomozov wrote: > Switching from embedded to detached signatures is a big change. This > feature needs to be thoroughly tested before embedded signatures will be > completely removed from the database. > > To help with detached signatures testing we enable it by default. But in > case if an user needs to go back to embedded signatures we add a config > option to reenable it - "UseEmbeddedSignatures". What is the purpose of this? Either signature source should be equivalent, and you should be able to trivially test this by creating a repo with unsigned packages, then bulk-signing the packages after they were repo-added. I don't believe that pacman should include such an end-user option purely to double-check whether a current feature actually works. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH v3 2/2] repo-add: use more libmakepkg to handle common compression routines
Currently the list of supported formats for an archive, is maintained in two places. And repo-add does not actually get updated. :( In the process, remove some of the logical duplication when calling bsdtar/compress_as. Signed-off-by: Eli Schwartz --- v3: use get_compression_command to do an early check if we can compress, thus preserving the old behavior of failing on invalid compression types. scripts/repo-add.sh.in | 32 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 160fd93a..7182d1b8 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -44,6 +44,7 @@ USE_COLOR='y' PREVENT_DOWNGRADE=0 # Import libmakepkg +source "$LIBRARY"/util/compress.sh source "$LIBRARY"/util/message.sh # ensure we have a sane umask set @@ -189,21 +190,13 @@ verify_signature() { } verify_repo_extension() { - local repofile=$1 - - case $repofile in - *.db.tar.gz) TAR_OPT="-z" ;; - *.db.tar.bz2) TAR_OPT="-j" ;; - *.db.tar.xz) TAR_OPT="-J" ;; - *.db.tar.zst) TAR_OPT="--zstd" ;; - *.db.tar.Z) TAR_OPT="-Z" ;; - *.db.tar) TAR_OPT="" ;; - *) error "$(gettext "'%s' does not have a valid database archive extension.")" \ - "$repofile" - exit 1 ;; - esac + local junk=() + if [[ $1 = *.db.tar* ]] && get_compression_command "$1" junk; then + return 0 + fi - printf '%s' "$TAR_OPT" + error "$(gettext "'%s' does not have a valid database archive extension.")" "$1" + exit 1 } # write an entry to the pacman database @@ -522,7 +515,6 @@ rotate_db() { } create_db() { - TAR_OPT=$(verify_repo_extension "$REPO_DB_FILE") # $LOCKFILE is already guaranteed to be absolute so this is safe dirname=${LOCKFILE%/*} @@ -532,13 +524,13 @@ create_db() { tempname=$dirname/.tmp.$filename pushd "$tmpdir/$repo" >/dev/null - if ( shopt -s nullglob; files=(*); (( ${#files[*]} )) ); then - bsdtar -c ${TAR_OPT} -f "$tempname" * - else + local files=(*) + if [[ ${files[*]} = '*' ]]; then # we have no packages remaining? zip up some emptyness warning "$(gettext "No packages remain, creating empty database.")" - bsdtar -c ${TAR_OPT} -f "$tempname" -T /dev/null + files=(-T /dev/null) fi + bsdtar -cf - "${files[@]}" | compress_as "$filename" > "$tempname" popd >/dev/null create_signature "$tempname" @@ -656,7 +648,7 @@ else LOCKFILE=$PWD/$REPO_DB_FILE.lck fi -verify_repo_extension "$REPO_DB_FILE" >/dev/null +verify_repo_extension "$REPO_DB_FILE" REPO_DB_PREFIX=${REPO_DB_FILE##*/} REPO_DB_PREFIX=${REPO_DB_PREFIX%.db.*} -- 2.28.0
[pacman-dev] [PATCH v3 1/2] libmakepkg: extend compress.sh to also permit checking validity
get_compression_command() can now be used to do upfront checks for whether a given extension is known to do something successfully. This is useful when writing tools in which an unknown compression type is a fatal error. Signed-off-by: Eli Schwartz --- v3: split commit, this first part implements a function useful for checking if we have a known good compression type scripts/libmakepkg/util/compress.sh.in | 53 +++--- 1 file changed, 40 insertions(+), 13 deletions(-) diff --git a/scripts/libmakepkg/util/compress.sh.in b/scripts/libmakepkg/util/compress.sh.in index 16420beb..d35a01fa 100644 --- a/scripts/libmakepkg/util/compress.sh.in +++ b/scripts/libmakepkg/util/compress.sh.in @@ -24,6 +24,7 @@ LIBMAKEPKG_UTIL_COMPRESS_SH=1 LIBRARY=${LIBRARY:-'@libmakepkgdir@'} source "$LIBRARY/util/message.sh" +source "$LIBRARY/util/pkgbuild.sh" # Wrapper around many stream compression formats, for use in the middle of a @@ -31,20 +32,46 @@ source "$LIBRARY/util/message.sh" compress_as() { # $1: final archive filename extension for compression type detection - local ext=".tar${1##*.tar}" + local cmd ext=${1#${1%.tar*}} + + if ! get_compression_command "$ext" cmd; then + warning "$(gettext "'%s' is not a valid archive extension.")" "${ext:-${1##*/}}" + cat + else + "${cmd[@]}" + fi +} + +# Retrieve the compression command for an archive extension, or cat for .tar, +# and save it to an existing array name. If the extension cannot be found, +# clear the array and return failure. +get_compression_command() { + local extarray ext=$1 outputvar=$2 + local resolvecmd=() fallback=() case "$ext" in - *.tar.gz) ${COMPRESSGZ[@]:-gzip -c -f -n} ;; - *.tar.bz2) ${COMPRESSBZ2[@]:-bzip2 -c -f} ;; - *.tar.xz) ${COMPRESSXZ[@]:-xz -c -z -} ;; - *.tar.zst) ${COMPRESSZST[@]:-zstd -c -z -q -} ;; - *.tar.lrz) ${COMPRESSLRZ[@]:-lrzip -q} ;; - *.tar.lzo) ${COMPRESSLZO[@]:-lzop -q} ;; - *.tar.Z) ${COMPRESSZ[@]:-compress -c -f} ;; - *.tar.lz4) ${COMPRESSLZ4[@]:-lz4 -q} ;; - *.tar.lz) ${COMPRESSLZ[@]:-lzip -c -f} ;; - *.tar) cat ;; - *) warning "$(gettext "'%s' is not a valid archive extension.")" \ - "$ext"; cat ;; + *.tar.gz) fallback=(gzip -c -f -n) ;; + *.tar.bz2) fallback=(bzip2 -c -f) ;; + *.tar.xz) fallback=(xz -c -z -) ;; + *.tar.zst) fallback=(zstd -c -z -q -) ;; + *.tar.lrz) fallback=(lrzip -q) ;; + *.tar.lzo) fallback=(lzop -q) ;; + *.tar.Z) fallback=(compress -c -f) ;; + *.tar.lz4) fallback=(lz4 -q) ;; + *.tar.lz) fallback=(lzip -c -f) ;; + *.tar) fallback=(cat) ;; + # do not respect unknown COMPRESS* env vars + *)array_build "$outputvar" resolvecmd; return 1 ;; esac + + ext=${ext#*.tar.} + if [[ -n $ext ]]; then + extarray="COMPRESS${ext^^}[@]" + resolvecmd=("${!extarray}") + fi + if (( ${#resolvecmd[@]} == 0 )); then + resolvecmd=("${fallback[@]}") + fi + + array_build "$outputvar" resolvecmd } -- 2.28.0
Re: [pacman-dev] Errors in pacman man pages
remote files\\&. " > "All instances of %u will be replaced with the download URL\\&. If present, " > "instances of %o will be replaced with the local filename, plus a \\(lq\\&." > "part\\(rq extension, which allows programs like wget to do file resumes " > "properly\\&." > > "This option is useful for users who experience problems with built-in HTTP/" > "FTP support, or need the more advanced proxy support that comes with " > "utilities like wget\\&." > -- > Man page: pacman.conf.5 > Issue: Incorrect markup of -U: "" → B<> > > "Set the signature verification level for installing packages using the \"-U" > "\" operation on a local file\\&. Uses the value from SigLevel as the default" > "\\&." > -- > Man page: pacman.conf.5 > Issue: Missing markup of syslog(): syslog() → B() > > "Log action messages through syslog()\\&. This will insert log entries into /" > "var/log/messages or equivalent\\&." > -- > Man page: pacman.conf.5 > Issue: Where did "%o" come from? The XferCommand command usage does not have > an %o > > "If set, an external program will be used to download all remote files\\&. " > "All instances of %u will be replaced with the download URL\\&. If present, " > "instances of %o will be replaced with the local filename, plus a \\(lq\\&." > "part\\(rq extension, which allows programs like wget to do file resumes " > "properly\\&." The rationale here seems to have been that %o is optional. Currently: XferCommand = /path/to/command %u Could be, perhaps: XferCommand = /path/to/command %u [--out %o] > -- > Man page: pacman.conf.5 > Issue: [options] should not be translated, wrap it with B<> > > "The I directive is valid in both the [options] and repository " > "sections\\&. If used in [options], it sets a default value for any " > "repository that does not provide the setting\\&." > -- > Man page: pacman-conf.8 > Issue: remote → repository > > "B<-r, --repo> EremoteE" > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] Fix srcinfo sometimes printing double newline at the end
On 6/25/20 7:51 PM, Denton Liu wrote: > Hi Allan, > > On Fri, Jun 26, 2020 at 09:43:39AM +1000, Allan McRae wrote: >> Please do not resend patches. We have a patch tracking system, which >> means patches are never lost. > > Sorry about that, coming from other mailing list projects, I didn't > realise that patches here are automatically tracked. Do you have a link > to the patch tracker? https://patchwork.archlinux.org/project/pacman/list/ > (Also, if possible, the v2 that I just sent should be used over the old > v2 as it contains an updated commit message without the typo and with a > sign-off.) I've marked your v2 patch as superseded. Today's v3 (mislabeled "v2" :p) is now the current iteration. >> I have given this patch a very low priority. That means with limited >> time available to me, I will likely review other patches first. But it >> will get considered at some stage. > > Thanks! > > -Denton At first glance it seems fine to me. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Adds a check to -Qo, so that pacman warns the user if the file does not exits
On 6/16/20 9:35 PM, Pedro Victor wrote: > That's true, thanks. I sent this patch because I mistyped a path in -Qo and > it only printed "file %mistyped_path is not owned by any package", without > warning me that the file did not exist. > So if this check is put somewhere after the file database query, it will > warn about mistyped paths to nonexistent files, right? Yes, that seems reasonable. Can you submit an updated patch? While you are at it, commit messages are usually in the imperative mood, e.g. "add a check [...]" rather than "adds a check". See e.g. https://git-scm.com/docs/SubmittingPatches#imperative-mood https://chris.beams.io/posts/git-commit/#imperative -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Adds a check to -Qo, so that pacman warns the user if the file does not exits
On 6/16/20 8:05 PM, Pedro Victor wrote: > From: Pedro Victor > > --- > src/pacman/query.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/src/pacman/query.c b/src/pacman/query.c > index 513ddb7a..784936b9 100644 > --- a/src/pacman/query.c > +++ b/src/pacman/query.c > @@ -209,6 +209,11 @@ static int query_fileowner(alpm_list_t *targets) > strcat(rpath + rlen, "/"); > } > > + if (access(rel_path, F_OK) == -1) { > + pm_printf(ALPM_LOG_ERROR, _("File does not exist: > %s\n"), rel_path); > + goto targcleanup; > + } > + Note that the current behavior is, if a file has been deleted and a warning is reported by 'pacman -Qkk', pacman -Qo /path/to/file will still report which package is *supposed* to it. > for(i = packages; i && (!found || is_dir); i = > alpm_list_next(i)) { > if(alpm_filelist_contains(alpm_pkg_get_files(i->data), > rel_path)) { > print_query_fileowner(rpath, i->data); > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH] doc/pacman.8: fix typo
Fixes FS#67000 Signed-off-by: Eli Schwartz --- doc/pacman.8.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/pacman.8.asciidoc b/doc/pacman.8.asciidoc index fee5aa15..476c16f3 100644 --- a/doc/pacman.8.asciidoc +++ b/doc/pacman.8.asciidoc @@ -141,7 +141,7 @@ Options guest system. See '\--sysroot' instead. *-v, \--verbose*:: - Output paths such as as the Root, Conf File, DB Path, Cache Dirs, etc. + Output paths such as the Root, Conf File, DB Path, Cache Dirs, etc. *\--arch* :: Specify an alternate architecture. -- 2.27.0
Re: [pacman-dev] [PATCH] makepkg: guard against undefined git pinned sources
On 6/10/20 8:51 PM, Allan McRae wrote: > On 26/5/20 1:52 pm, Eli Schwartz wrote: >> If something like source=(..."#commit=") is used, e.g. due to failed >> variable expansion, we try to check out an empty refspec as nothing at >> all, and end up just running "git checkout". This happens because we >> fail at variable expansion too -- so let's quote our variables properly >> and make sure git sees this as an empty refspec, so it can error out. >> >> Also make sure it is interpreted as a ref instead of a path. >> >> Signed-off-by: Eli Schwartz >> --- >> >> This ensures that something like https://bugs.archlinux.org/task/66729 >> cannot happen again. >> > > Patch good. > > Worth checking if this can happen with other VCS too. Ironically (considering git is the primary VCS which we use and test) it seems like all the other VCSes quote their args. Except for svn, which runs svn update -r ${ref} and the -r requires an argument and fails if it is not provided. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH] makepkg/repo-add: do not accept public-only keys for signing
If it's not listed by --list-secret-key we don't care if it has been imported into your keyring, it's unusable. And you might not have a private key at all in the no-keyid-specified case. Signed-off-by: Eli Schwartz --- Overlaps previous patch to fix GPGKEY handling. scripts/makepkg.sh.in | 2 +- scripts/repo-add.sh.in | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cc8de5aa..9e75ef17 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1293,7 +1293,7 @@ fi # check if gpg signature is to be created and if signing key is valid if { [[ -z $SIGNPKG ]] && check_buildenv "sign" "y"; } || [[ $SIGNPKG == 'y' ]]; then SIGNPKG='y' - if ! gpg --list-key ${GPGKEY:+"$GPGKEY"} &>/dev/null; then + if ! gpg --list-secret-key ${GPGKEY:+"$GPGKEY"} &>/dev/null; then if [[ ! -z $GPGKEY ]]; then error "$(gettext "The key %s does not exist in your keyring.")" "${GPGKEY}" else diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 272d8d22..160fd93a 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -137,7 +137,7 @@ check_gpg() { fi if (( ! VERIFY )); then - if ! gpg --list-key ${GPGKEY:+"$GPGKEY"} &>/dev/null; then + if ! gpg --list-secret-key ${GPGKEY:+"$GPGKEY"} &>/dev/null; then if [[ ! -z $GPGKEY ]]; then error "$(gettext "The key ${GPGKEY} does not exist in your keyring.")" elif (( ! KEY )); then -- 2.27.0
[pacman-dev] [PATCH v2] makepkg/repo-add: handle GPGKEY with spaces
We pass this to gpg -u and this gpg option can accept a number of different formats, not just the historical hexadecimal fingerprint we assumed. We should not barf hard if a format is used which happens to contain spaces. This also fixes a validation bug. When we initially check if the desired key is available, we don't quote spaces, so gpg goes ahead and treats each space-separated string as a *different key* to search for, returning partial matches, and returning success if at least one key is found. But gpg --detach-sign -u will certainly not accept multiple keys! Fixes FS#66949 Signed-off-by: Eli Schwartz --- v2: fix case of GPGKEY="" with signing enabled reporting that no keys exist in the keyring. Only expand the quoted GPGKEY if it is non-empty. scripts/libmakepkg/integrity/generate_signature.sh.in | 6 +++--- scripts/makepkg.sh.in | 2 +- scripts/repo-add.sh.in| 8 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/scripts/libmakepkg/integrity/generate_signature.sh.in b/scripts/libmakepkg/integrity/generate_signature.sh.in index aec96c03..748087c2 100644 --- a/scripts/libmakepkg/integrity/generate_signature.sh.in +++ b/scripts/libmakepkg/integrity/generate_signature.sh.in @@ -29,12 +29,12 @@ create_signature() { local ret=0 local filename="$1" - local SIGNWITHKEY="" + local SIGNWITHKEY=() if [[ -n $GPGKEY ]]; then - SIGNWITHKEY="-u ${GPGKEY}" + SIGNWITHKEY=(-u "${GPGKEY}") fi - gpg --detach-sign --use-agent ${SIGNWITHKEY} --no-armor "$filename" &>/dev/null || ret=$? + gpg --detach-sign --use-agent "${SIGNWITHKEY[@]}" --no-armor "$filename" &>/dev/null || ret=$? if (( ! ret )); then diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 7261fb2c..cc8de5aa 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1293,7 +1293,7 @@ fi # check if gpg signature is to be created and if signing key is valid if { [[ -z $SIGNPKG ]] && check_buildenv "sign" "y"; } || [[ $SIGNPKG == 'y' ]]; then SIGNPKG='y' - if ! gpg --list-key ${GPGKEY} &>/dev/null; then + if ! gpg --list-key ${GPGKEY:+"$GPGKEY"} &>/dev/null; then if [[ ! -z $GPGKEY ]]; then error "$(gettext "The key %s does not exist in your keyring.")" "${GPGKEY}" else diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 545c2929..272d8d22 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -137,7 +137,7 @@ check_gpg() { fi if (( ! VERIFY )); then - if ! gpg --list-key ${GPGKEY} &>/dev/null; then + if ! gpg --list-key ${GPGKEY:+"$GPGKEY"} &>/dev/null; then if [[ ! -z $GPGKEY ]]; then error "$(gettext "The key ${GPGKEY} does not exist in your keyring.")" elif (( ! KEY )); then @@ -155,11 +155,11 @@ create_signature() { local ret=0 msg "$(gettext "Signing database '%s'...")" "${dbfile##*/.tmp.}" - local SIGNWITHKEY="" + local SIGNWITHKEY=() if [[ -n $GPGKEY ]]; then - SIGNWITHKEY="-u ${GPGKEY}" + SIGNWITHKEY=(-u "${GPGKEY}") fi - gpg --detach-sign --use-agent --no-armor ${SIGNWITHKEY} "$dbfile" &>/dev/null || ret=$? + gpg --detach-sign --use-agent --no-armor "${SIGNWITHKEY[@]}" "$dbfile" &>/dev/null || ret=$? if (( ! ret )); then msg2 "$(gettext "Created signature file '%s'")" "${dbfile##*/.tmp.}.sig" -- 2.27.0
[pacman-dev] [PATCH] makepkg/repo-add: handle GPGKEY with spaces
We pass this to gpg -u and this gpg option can accept a number of different formats, not just the historical hexadecimal fingerprint we assumed. We should not barf hard if a format is used which happens to contain spaces. This also fixes a validation bug. When we initially check if the desired key is available, we don't quote spaces, so gpg goes ahead and treats each space-separated string as a *different key* to search for, returning partial matches, and returning success if at least one key is found. But gpg --detach-sign -u will certainly not accept multiple keys! Fixes FS#66949 Signed-off-by: Eli Schwartz --- scripts/libmakepkg/integrity/generate_signature.sh.in | 6 +++--- scripts/makepkg.sh.in | 2 +- scripts/repo-add.sh.in| 8 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/scripts/libmakepkg/integrity/generate_signature.sh.in b/scripts/libmakepkg/integrity/generate_signature.sh.in index aec96c03..748087c2 100644 --- a/scripts/libmakepkg/integrity/generate_signature.sh.in +++ b/scripts/libmakepkg/integrity/generate_signature.sh.in @@ -29,12 +29,12 @@ create_signature() { local ret=0 local filename="$1" - local SIGNWITHKEY="" + local SIGNWITHKEY=() if [[ -n $GPGKEY ]]; then - SIGNWITHKEY="-u ${GPGKEY}" + SIGNWITHKEY=(-u "${GPGKEY}") fi - gpg --detach-sign --use-agent ${SIGNWITHKEY} --no-armor "$filename" &>/dev/null || ret=$? + gpg --detach-sign --use-agent "${SIGNWITHKEY[@]}" --no-armor "$filename" &>/dev/null || ret=$? if (( ! ret )); then diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 7261fb2c..84150cff 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1293,7 +1293,7 @@ fi # check if gpg signature is to be created and if signing key is valid if { [[ -z $SIGNPKG ]] && check_buildenv "sign" "y"; } || [[ $SIGNPKG == 'y' ]]; then SIGNPKG='y' - if ! gpg --list-key ${GPGKEY} &>/dev/null; then + if ! gpg --list-key "${GPGKEY}" &>/dev/null; then if [[ ! -z $GPGKEY ]]; then error "$(gettext "The key %s does not exist in your keyring.")" "${GPGKEY}" else diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 545c2929..b1b1b2b7 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -137,7 +137,7 @@ check_gpg() { fi if (( ! VERIFY )); then - if ! gpg --list-key ${GPGKEY} &>/dev/null; then + if ! gpg --list-key "${GPGKEY}" &>/dev/null; then if [[ ! -z $GPGKEY ]]; then error "$(gettext "The key ${GPGKEY} does not exist in your keyring.")" elif (( ! KEY )); then @@ -155,11 +155,11 @@ create_signature() { local ret=0 msg "$(gettext "Signing database '%s'...")" "${dbfile##*/.tmp.}" - local SIGNWITHKEY="" + local SIGNWITHKEY=() if [[ -n $GPGKEY ]]; then - SIGNWITHKEY="-u ${GPGKEY}" + SIGNWITHKEY=(-u "${GPGKEY}") fi - gpg --detach-sign --use-agent --no-armor ${SIGNWITHKEY} "$dbfile" &>/dev/null || ret=$? + gpg --detach-sign --use-agent --no-armor "${SIGNWITHKEY[@]}" "$dbfile" &>/dev/null || ret=$? if (( ! ret )); then msg2 "$(gettext "Created signature file '%s'")" "${dbfile##*/.tmp.}.sig" -- 2.27.0
Re: [pacman-dev] makepkg: package_...() can export variables to each other
On 6/5/20 3:59 AM, Allan McRae wrote: > On 5/6/20 5:39 pm, Erich Eckner wrote: >> Hi, >> >> I stumbled upon the fact, that variables set in one package_...() >> function of a split package are accessible by following package_...() >> functions. Is this by design or may I try to provide a patch to restore >> env after the call to package_...()? >> Note: packaging-relevant variables (e.g. depends_x86_64) are cleaned >> across function calls. >> >> Simple example: >> >> --8<--8<--8<-- >> arch=(any) >> pkgbase=test >> pkgname=(ta tb) >> pkgver=0 >> pkgrel=1 >> >> package_ta() { >> _test='hi, there!' >> } >> >> package_tb() { >> echo "$_test" >> } >> -->8-->8-->8-- >> > > PKGBUILDs are bash. Bash scope rules apply. ... with the caveat that makepkg --nobuild && makepkg --noextract will run prepare() and build() in separate program invocations, so you cannot *depend* on variables being persisted. And all package_*() functions are run -- together -- under fakeroot in a separate bash process, so they are separated in scope from non-package_*() functions. The packaging-relevant variables are special, we reset them precisely because they're documented to only be local to the package that defines them. For all other variables, as Allan said it's your job to scope them as you wish. People usually declare them with the "local" builtin. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] srcinfo.sh: remove trailing newline
On 6/4/20 1:00 AM, Erich Eckner wrote: > In case my voice counts anything in this discussion: I would prefer the > empty lines between the sections and at the end to stay. I regularly > parse .SRCINFOs (or the output of `makepkg --printsrcinfo`) and > terminate parsing of sections on the empty line (think along the lines > `sed '/^\S/,/^$/ {...}'`). I should probably clarify that the empty lines as section separators are part of the current format specification and as far as I'm concerned, removing them is simply not up for discussion. Any proposed patch *must* retain those empty lines. Your use case is a good example of why. And I have no reason to think you're the only person in the entire userbase who is parsing srcinfo content with this assumption. Removing the *final* trailing newline could in theory be done (any srcinfo parser would presumably terminate at EOF anyway) and that, I'm "merely unconvinced" about implementing. I still don't think a compelling case has been made for it, but I'm willing to be convinced. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] srcinfo.sh: remove trailing newline
On 6/3/20 8:04 PM, Eli Schwartz wrote: > On 6/3/20 7:26 PM, Denton Liu wrote: >> When the .SRCINFO file is generated via `makepkg --printsrcinfo`, each >> section is concluded with an empty line. This means that at the end of >> the file, an empty line remains. When running `git diff --check`, Git >> will complain about this as a whitespace error, saying "new blank line >> at EOF." > > git diff --check isn't necessarily our problem, though... > > But it will only be reported once, the very first time you ever commit > this .SRCINFO Also FWIW, I use git diff --check in my pre-commit githook for AUR packages, but I also generate the .SRCINFO in that very hook, AFTER I check for whitespace issues. Hence I don't see whitespace issues for the .SRCINFO file, even for the very first commit. So I'm really not motivated to change this. For more details see https://github.com/eli-schwartz/aurpublish/#hooks -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] srcinfo.sh: remove trailing newline
On 6/3/20 7:26 PM, Denton Liu wrote: > When the .SRCINFO file is generated via `makepkg --printsrcinfo`, each > section is concluded with an empty line. This means that at the end of > the file, an empty line remains. When running `git diff --check`, Git > will complain about this as a whitespace error, saying "new blank line > at EOF." git diff --check isn't necessarily our problem, though... But it will only be reported once, the very first time you ever commit this .SRCINFO > Remove the empty line after sections. Replace the empty echo with a > placeholder `true` call in case in the future, we do want to close the > section with something. Now you've also removed the blank line in the middle of the file, separating the pkgbase section from each pkgname section. That is more than just the blank line terminating the final pkgname section. > --- > scripts/libmakepkg/srcinfo.sh.in | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/libmakepkg/srcinfo.sh.in > b/scripts/libmakepkg/srcinfo.sh.in > index 6e783279..e7b5c4be 100644 > --- a/scripts/libmakepkg/srcinfo.sh.in > +++ b/scripts/libmakepkg/srcinfo.sh.in > @@ -31,7 +31,7 @@ srcinfo_open_section() { > } > > srcinfo_close_section() { > - echo > + true # nothing to be done > } > > srcinfo_write_attr() { > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH v2 1/2] makepkg: correctly handle missing download clients
This was broken in commit 882e707e40bbade0111cf3bdedbdac4d4b70453b, which changed 'plain()' messages to go to stdout, which was then captured as the download client in question: cmdline=("Aborting..."). The result was a very confusing error message e.g. /usr/share/makepkg/source/file.sh: line 72: $'\E[1m': command not found or with makepkg --nocolor: /usr/share/makepkg/source/file.sh: line 72: Aborting...: command not found The problem here is that we checked to see if an asynchronous subshell, in our case <(...), failed, by checking if its captured stdout is non-empty. Which is terrible, and also a limitation of old bash. But bash 4.4 can use wait $! to retrieve the return value of an asynchronous subshell. Now we target that as our minimum, we can sanely handle errors in such functions. Losing error messages on stdout by capturing them in a variable instead of printing them, continues to be a problem, but this will be fixed systematically in a later commit. Signed-off-by: Eli Schwartz --- v2: split out the plain() function redirection into its own patch, which will be handled more consistently. scripts/libmakepkg/source/file.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/libmakepkg/source/file.sh.in b/scripts/libmakepkg/source/file.sh.in index 819320c2..2b804564 100644 --- a/scripts/libmakepkg/source/file.sh.in +++ b/scripts/libmakepkg/source/file.sh.in @@ -42,7 +42,7 @@ download_file() { # find the client we should use for this URL local -a cmdline IFS=' ' read -a cmdline < <(get_downloadclient "$proto") - (( ${#cmdline[@]} )) || exit + wait $! || exit local filename=$(get_filename "$netfile") local url=$(get_url "$netfile") -- 2.26.2
[pacman-dev] [PATCH v2 2/2] libmakepkg: fix regression in sending plain() output to stderr
In commit 882e707e40bbade0111cf3bdedbdac4d4b70453b we changed message output to go to stdout by default, unless it was an error. The plain() function doesn't *look* like an error function, but in practice it was -- it's used to continue multiline messages, and all in-tree uses were for warning/error. This is a problem both because we're sending output to the wrong place, and because in some cases, we were performing error logging from a function which would otherwise return a value to be captured in a variable using command substution. Fix this and straighten out the API by providing two functions: one for continuing msg output, and one which wraps this by sending output to stderr, for continuing error output. Change all callers to use the second function. Signed-off-by: Eli Schwartz --- scripts/libmakepkg/executable/vcs.sh.in | 2 +- .../libmakepkg/integrity/verify_signature.sh.in | 2 +- scripts/libmakepkg/source/bzr.sh.in | 8 scripts/libmakepkg/source/file.sh.in | 4 ++-- scripts/libmakepkg/source/git.sh.in | 14 +++--- scripts/libmakepkg/source/hg.sh.in | 8 scripts/libmakepkg/source/svn.sh.in | 4 ++-- scripts/libmakepkg/util/config.sh.in | 2 +- scripts/libmakepkg/util/message.sh.in| 8 scripts/libmakepkg/util/source.sh.in | 4 ++-- scripts/libmakepkg/util/util.sh.in | 2 +- scripts/makepkg.sh.in| 16 12 files changed, 41 insertions(+), 33 deletions(-) diff --git a/scripts/libmakepkg/executable/vcs.sh.in b/scripts/libmakepkg/executable/vcs.sh.in index 6eb93fae..436b82db 100644 --- a/scripts/libmakepkg/executable/vcs.sh.in +++ b/scripts/libmakepkg/executable/vcs.sh.in @@ -43,7 +43,7 @@ get_vcsclient() { # if we didn't find an client, return an error if [[ -z $client ]]; then error "$(gettext "Unknown download protocol: %s")" "$proto" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_CONFIG_ERROR fi diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in index a1bf7f1c..2be5c493 100644 --- a/scripts/libmakepkg/integrity/verify_signature.sh.in +++ b/scripts/libmakepkg/integrity/verify_signature.sh.in @@ -112,7 +112,7 @@ check_pgpsigs() { if (( warnings )); then warning "$(gettext "Warnings have occurred while verifying the signatures.")" - plain "$(gettext "Please make sure you really trust them.")" + plainerr "$(gettext "Please make sure you really trust them.")" fi } diff --git a/scripts/libmakepkg/source/bzr.sh.in b/scripts/libmakepkg/source/bzr.sh.in index 1227c739..d8e47185 100644 --- a/scripts/libmakepkg/source/bzr.sh.in +++ b/scripts/libmakepkg/source/bzr.sh.in @@ -52,7 +52,7 @@ download_bzr() { msg2 "$(gettext "Branching %s...")" "${displaylocation}" if ! bzr branch "$url" "$dir" --no-tree --use-existing-dir; then error "$(gettext "Failure while branching %s")" "${displaylocation}" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi elif (( ! HOLDVER )); then @@ -83,7 +83,7 @@ extract_bzr() { ;; *) error "$(gettext "Unrecognized reference: %s")" "${fragment}" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 esac fi @@ -98,12 +98,12 @@ extract_bzr() { cd_safe "${dir##*/}" if ! (bzr pull "$dir" -q --overwrite -r "$rev" && bzr clean-tree -q --detritus --force); then error "$(gettext "Failure while updating working copy of %s %s repo")" "${repo}" "bzr" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi elif ! bzr checkout "$dir" -r "$rev"; then error "$(gettext "Failure while creating working copy of %s %s repo")" "${repo}" "bzr" -
[pacman-dev] [PATCH] makepkg: guard against undefined git pinned sources
If something like source=(..."#commit=") is used, e.g. due to failed variable expansion, we try to check out an empty refspec as nothing at all, and end up just running "git checkout". This happens because we fail at variable expansion too -- so let's quote our variables properly and make sure git sees this as an empty refspec, so it can error out. Also make sure it is interpreted as a ref instead of a path. Signed-off-by: Eli Schwartz --- This ensures that something like https://bugs.archlinux.org/task/66729 cannot happen again. scripts/libmakepkg/source/git.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/libmakepkg/source/git.sh.in b/scripts/libmakepkg/source/git.sh.in index aee944f7..a29be3c5 100644 --- a/scripts/libmakepkg/source/git.sh.in +++ b/scripts/libmakepkg/source/git.sh.in @@ -125,7 +125,7 @@ extract_git() { fi if [[ $ref != "origin/HEAD" ]] || (( updating )) ; then - if ! git checkout --force --no-track -B makepkg $ref; then + if ! git checkout --force --no-track -B makepkg "$ref" --; then error "$(gettext "Failure while creating working copy of %s %s repo")" "${repo}" "git" plain "$(gettext "Aborting...")" exit 1 -- 2.26.2
[pacman-dev] [PATCH] build: add libintl dependency to meson and the .pc file
In order to use gettext on systems where it is not part of libc, the correct linker flags are needed in libalpm.pc (for static compilation). This has never been the case. The new meson build system currently only checks for ngettext in libc, but does not fall back to searching for the existence of -lintl; add it to the libalpm dependencies. Signed-off-by: Eli Schwartz --- Fixes build on MSYS2, and possibly others. Part of https://github.com/msys2/MSYS2-packages/pull/1951 lib/libalpm/libalpm.pc.in | 2 +- meson.build | 8 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/libalpm/libalpm.pc.in b/lib/libalpm/libalpm.pc.in index 126a6e6a..da7f96aa 100644 --- a/lib/libalpm/libalpm.pc.in +++ b/lib/libalpm/libalpm.pc.in @@ -10,4 +10,4 @@ Version: @LIB_VERSION@ Requires.private: libarchive @pc_crypto@ @pc_libcurl@ @pc_gpgme@ Cflags: -I${includedir} @LFS_CFLAGS@ Libs: -L${libdir} -lalpm -Libs.private: @LIBS@ @pc_gpgme_libs@ +Libs.private: @LIBS@ @pc_gpgme_libs@ @LIBINTL@ diff --git a/meson.build b/meson.build index 331f02b8..aa3bd4d9 100644 --- a/meson.build +++ b/meson.build @@ -77,9 +77,13 @@ conf.set_quoted('CACHEDIR', join_paths(LOCALSTATEDIR, 'cache/pacman/pkg/')) conf.set_quoted('HOOKDIR', join_paths(SYSCONFDIR, 'pacman.d/hooks/')) conf.set_quoted('ROOTDIR', ROOTDIR) +libintl = dependency('', required: false) if get_option('i18n') if not cc.has_function('ngettext') -error('ngettext not found but NLS support requested') +libintl = cc.find_library('intl', required : false, static: get_option('buildstatic')) +if not libintl.found() + error('ngettext not found but NLS support requested') +endif endif conf.set('ENABLE_NLS', 1) endif @@ -301,7 +305,7 @@ libcommon = static_library( include_directories : includes, install : false) -alpm_deps = [crypto_provider, libarchive, libcurl, gpgme] +alpm_deps = [crypto_provider, libarchive, libcurl, libintl, gpgme] libalpm_a = static_library( 'alpm_objlib', -- 2.26.2
[pacman-dev] [PATCH] makepkg: correctly handle missing download clients
This was broken in commit 882e707e40bbade0111cf3bdedbdac4d4b70453b, which changed 'plain()' messages to go to stdout, which was then captured as the download client in question: cmdline=("Aborting..."). The result was a very confusing error message e.g. /usr/share/makepkg/source/file.sh: line 72: $'\E[1m': command not found or with makepkg --nocolor: /usr/share/makepkg/source/file.sh: line 72: Aborting...: command not found Solve this on two levels: - redirect this error() continuation to stderr so the user sees it. - catch erroring returns in get_downloadclient and propagate them bash 4.4 can use wait $! to retrieve the return value of an asynchronous subshell such as <(...), which means, now we target that as our minimum, we can sanely handle errors in such functions. Signed-off-by: Eli Schwartz --- Actually, maybe every use of plain() needs to do this. Or else make plain() do this by default. But it's technically used to "continue the previous message", and there's no guarantee it merits going to stderr, even though every time we use plain(), it does. What to do... scripts/libmakepkg/source/file.sh.in | 1 + scripts/libmakepkg/util/source.sh.in | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/libmakepkg/source/file.sh.in b/scripts/libmakepkg/source/file.sh.in index 819320c2..28f373fb 100644 --- a/scripts/libmakepkg/source/file.sh.in +++ b/scripts/libmakepkg/source/file.sh.in @@ -42,6 +42,7 @@ download_file() { # find the client we should use for this URL local -a cmdline IFS=' ' read -a cmdline < <(get_downloadclient "$proto") + wait $! || exit (( ${#cmdline[@]} )) || exit local filename=$(get_filename "$netfile") diff --git a/scripts/libmakepkg/util/source.sh.in b/scripts/libmakepkg/util/source.sh.in index e0490661..1bf5b814 100644 --- a/scripts/libmakepkg/util/source.sh.in +++ b/scripts/libmakepkg/util/source.sh.in @@ -165,7 +165,7 @@ get_downloadclient() { if [[ ! -x $program ]]; then local baseprog="${program##*/}" error "$(gettext "The download program %s is not installed.")" "$baseprog" - plain "$(gettext "Aborting...")" + plain "$(gettext "Aborting...")" >&2 exit 1 # $E_MISSING_PROGRAM fi -- 2.26.2
[pacman-dev] [PATCH] makepkg: libprovides: don't provide both versioned and unversioned sonames
If multiple files match the pattern libfoo.so*, we want to check each of them and see if they are shared libraries, and if so, if they have versions attached. But some packages can have both shared libraries and random files which match the filename pattern. This is true at least for files in /usr/share/gdb/auto-load/, which must match the filename they are paired with, followed by "-gdb.py" (or some other gdb scripting ext), but definitely don't contain a shared library. In this case, we don't want to double-report the library in the generated provides. It's also possible (probably) for a package to provide a versioned as well as an unversioned shared library, but in such cases a single provides entry is sufficient to cover both cases (and the libdepends for the depending package would contain an unversioned dependency). Solve this by keeping track of whether we have added a versioned soname provides already, and then only adding a maximum of one unversioned provides *iff* there isn't a versioned one yet. Signed-off-by: Eli Schwartz --- Notes: Currently we only have gdb/auto-load/ scripts for shared libraries in: - gcc - glib2 - gstreamer - efl In the first 3 cases, libprovides is entirely useless anyway. libstdc++ is perpetually stable (backwards-compatible ABI) just like glibc. glib2 actually does use libprovides, despite that it makes no sense for glib2 to libprovide anything, as the glib2 developers have this thing where they always use a soname of "0" and bump ABI versions by changing libglib-2.0.so to libglib-3.0.so -- as a result if you want to track dependencies you need to change to a different soname manually, and might as well just depend on the package itself (which already has a version in the pkgname). (gstreamer is in the same boat as glib2 because it is part of the same ecosystem.) efl doesn't currently libprovide "libeo.so", but if it did, then the issue would exist and be fixed by this patch. And more generally, any project wishing to implement gdb/auto-load/ scripts could have this issue, so it makes sense to make it work nicely. scripts/makepkg.sh.in | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index d1416d15..3f746b77 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -516,9 +516,10 @@ find_libdepends() { find_libprovides() { - local p libprovides missing + local p newp libprovides missing for p in "${provides[@]}"; do missing=0 + newp=() case "$p" in *.so) mapfile -t filename < <(find "$pkgdir" -type f -name $p\*) @@ -531,7 +532,6 @@ find_libprovides() { local sofile=$(LC_ALL=C readelf -d "$fn" 2>/dev/null | sed -n 's/.*Library soname: \[\(.*\)\].*/\1/p') if [[ -z "$sofile" ]]; then warning "$(gettext "Library listed in %s is not versioned: %s")" "'provides'" "$p" - libprovides+=("$p") continue fi @@ -541,25 +541,25 @@ find_libprovides() { # extract the library major version local soversion="${sofile##*\.so\.}" - libprovides+=("${p}=${soversion}-${soarch}") + newp+=("${p}=${soversion}-${soarch}") else warning "$(gettext "Library listed in %s is not a shared object: %s")" "'provides'" "$p" - libprovides+=("$p") fi done else - libprovides+=("$p") missing=1 fi ;; - *) - libprovides+=("$p") - ;; esac if (( missing )); then warning "$(gettext "Cannot find library list
Re: [pacman-dev] [PATCH v2] Swap alpm_db_update() implementation to multiplexed version
On 5/11/20 10:27 AM, guilla...@manjaro.org wrote: > Hi Allan, Hi Anatol, > > I was looking to this new multiplexed implementation and I found some > problems: > 1) This implementation can lead to download databases from different > servers, I think it can't be a problem if they are not synced This should already be the case in the single-download version too, no? > 2) Differents download payloads are created for databases and its > signature files, this can this time lead to download the database and > its signature from differents server, I'm sure it's not wanted. > 3) Because signature files have their own download payloads, if the > signature is missing on a the first server (e.g. database signature is > optional), all next servers will be tried and all will give the same error. re 2) I cannot remember offhand if this was already the case, but anyway it's not impossible for one to download the database successfully from one mirror, then get a sudden network issue on the signature. Falling back to a second server for optional signatures will just lead to download errors (which are ignored for sigs), and for required signatures it will fail the signature check the same way failure to download the signature at all would fail, because one way or another we *need* an updated signature. re both 2) and 3) I guess this could be optimized in the database download handling to abort on the first 404 but fall through to additional servers for connection failures. I'm not sure whether this would be a good thing... how would we detect mirrors that can be connected to, but no longer host any pacman repos? Checking 5 mirrors for 404 errors isn't much of an issue. I guess it could also solve this error I got: error: failed retrieving file 'core.db.sig' from mirror.epiphyte.network : Could not resolve host: mirror.epiphyte.network error: failed retrieving file 'testing.db.sig' from mirror.epiphyte.network : Could not resolve host: mirror.epiphyte.network error: failed retrieving file 'extra.db.sig' from mirror.epiphyte.network : Could not resolve host: mirror.epiphyte.network error: failed retrieving file 'community-testing.db.sig' from mirror.epiphyte.network : Could not resolve host: mirror.epiphyte.network error: failed retrieving file 'community.db.sig' from mirror.epiphyte.network : Could not resolve host: mirror.epiphyte.network error: failed retrieving file 'multilib.db.sig' from mirror.epiphyte.network : Could not resolve host: mirror.epiphyte.network error: failed retrieving file 'multilib-testing.db.sig' from mirror.epiphyte.network : Could not resolve host: mirror.epiphyte.network But I'd already solved this one by removing the dead mirror from the bottom of my mirrorlist (it used to be a pretty good fallback one). When all mirrors fail in the more normal manner at retrieving a signature, there's no UI results. > I really wanted to suggest a patch but I didn't find a easy way > (understand a small patch) to fix it and I would like your opinion on > point 1) first. > > Regards, > Guillaume. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Use absolute path for pacman and pacman-conf in makepkg
On 5/11/20 9:01 AM, wwijs...@live.nl wrote: > Right now an unpatched pacman almost works for my use case, but there > still is one issue with building it. Currently ninja will install the > bash-completion scripts system wide, regardless of which user runs > ninja install or what the prefix is. Only if bash-completion is not > available, will the scripts be installed inside the prefix. The code > which makes this happen is here: > https://git.archlinux.org/pacman.git/tree/meson.build#n46 > > This is why I submitted the patch which makes bash-completion optional. > It may need a different solution, though. Would you take a patch which > adds an option to install the bash-completion scripts inside the > prefix? I'd prefer not to maintain any private patches if that's > possible. https://git.archlinux.org/users/eschwartz/pacman.git/commit/?h=queue2=b4e3e1c7d93463dffdbb63ff78008df0f57780db It's simple enough to do, I've just been trying to decide whether I think it is the right solution before proposing it. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH] libmakepkg/strip: don't re-add the same debug source multiple times
It's either a waste of work, or triggers edge cases in some packages (like coreutils-8.31) where the source file is readonly and cp gets a permission denied error trying to overwrite it with an identical copy of itself. Also while we are at it, make the variable names be something readable, because I could barely tell what this was doing while editing it. Signed-off-by: Eli Schwartz --- scripts/libmakepkg/tidy/strip.sh.in | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in index 2b6f732d..868b96f3 100644 --- a/scripts/libmakepkg/tidy/strip.sh.in +++ b/scripts/libmakepkg/tidy/strip.sh.in @@ -56,11 +56,14 @@ strip_file() { fi # copy source files to debug directory - local f t + local file dest t while IFS= read -r t; do - f=${t/${dbgsrcdir}/"$srcdir"} - mkdir -p "${dbgsrc/"$dbgsrcdir"/}${t%/*}" - cp -- "$f" "${dbgsrc/"$dbgsrcdir"/}$t" + file=${t/${dbgsrcdir}/"$srcdir"} + dest="${dbgsrc/"$dbgsrcdir"/}$t" + if ! [[ -f $dest ]]; then + mkdir -p "${dest%/*}" + cp -- "$file" "$dest" + fi done < <(source_files "$binary") # copy debug symbols to debug directory -- 2.26.2
Re: [pacman-dev] [PATCH] makepkg: deterministic PKGINFO libprovides for multiple library versions
On 5/10/20 6:45 PM, anthr...@archlinux.org wrote: > From: Levente Polyak > > While iterating over the provides array, the find call for locating a > shared library may result in listing multiple entries which by itself > does not produce a stable deterministic order and may vary depending on > the underlying filesystem. > To provide a stable listing and a reproducible .PKGINFO file the result > of find is piped to sort with a static LC_ALL=C localisation. Wait, what. Do we have packages with libprovides providing multiple versions of a shared library? I mean yes, given this possibility it does seem like we'd need to sort them, but still... why does this exist? :p -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH 3/3] pacman-conf: fix half-baked support for ILoveCandy
On 5/10/20 4:06 AM, Allan McRae wrote: > On 10/5/20 2:32 pm, Eli Schwartz wrote: >> This was only partially implemented in the original implementation. >> `pacman-conf | grep ILoveCandy` would tell you if it was set, but >> querying directly by name would not. >> > > OK. > > Note "half-baked" can have negative connotations, while "incomplete" > specifies that issue without being accusatory. Feel free to exchange the commit message with that text, and chalk it up to late-night coding. :D -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH 3/3] pacman-conf: fix half-baked support for ILoveCandy
This was only partially implemented in the original implementation. `pacman-conf | grep ILoveCandy` would tell you if it was set, but querying directly by name would not. Signed-off-by: Eli Schwartz --- src/pacman/pacman-conf.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index d76c0985..463badf1 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -379,6 +379,8 @@ static int list_directives(void) show_bool("VerbosePkgLists", config->verbosepkglists); } else if(strcasecmp(i->data, "DisableDownloadTimeout") == 0) { show_bool("DisableDownloadTimeout", config->disable_dl_timeout); + } else if(strcasecmp(i->data, "ILoveCandy") == 0) { + show_bool("ILoveCandy", config->chomp); } else if(strcasecmp(i->data, "NoProgressBar") == 0) { show_bool("NoProgressBar", config->noprogressbar); -- 2.26.2
[pacman-dev] [PATCH 2/3] log invalid conf settings as an error
This is not a warning, _parse_options() returns failure without even parsing further lines and the attempted pacman/pacman-conf program execution immediately aborts. Warnings are for when e.g. later on if we don't recognize a setting at all, we skip over it and have enough confidence in this to continue executing the program. The current implementation results in pacman-conf aborting with: warning: config file /etc/pacman.conf, line 60: invalid value for 'ParallelDownloads' : '2.5' error parsing '/etc/pacman.conf' or pacman -Syu aborting with the entirely more cryptic: warning: config file /etc/pacman.conf, line 59: invalid value for 'ParallelDownloads' : '2.5' and this isn't just a problem for the newly added ParallelDownloads setting, either, you could get the same problem if you specified a broken XferCommand, but that's harder as it's more accepting of input and you probably don't hit this except with unbalanced quotes. Signed-off-by: Eli Schwartz --- src/pacman/conf.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index ac5a5329..3a3ef605 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -665,7 +665,7 @@ static int _parse_options(const char *key, char *value, } else if(strcmp(key, "XferCommand") == 0) { char **c; if((config->xfercommand_argv = wordsplit(value)) == NULL) { - pm_printf(ALPM_LOG_WARNING, + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: invalid value for '%s' : '%s'\n"), file, linenum, "XferCommand", value); return 1; @@ -717,21 +717,21 @@ static int _parse_options(const char *key, char *value, err = parse_number(value, ); if(err) { - pm_printf(ALPM_LOG_WARNING, + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: invalid value for '%s' : '%s'\n"), file, linenum, "ParallelDownloads", value); return 1; } if(number < 1) { - pm_printf(ALPM_LOG_WARNING, + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: value for '%s' has to be positive : '%s'\n"), file, linenum, "ParallelDownloads", value); return 1; } if(number > INT_MAX) { - pm_printf(ALPM_LOG_WARNING, + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: value for '%s' is too large : '%s'\n"), file, linenum, "ParallelDownloads", value); return 1; -- 2.26.2
[pacman-dev] [PATCH 1/3] pacman-conf: add support for new ParallelDownloads config option
This was forgotten in the initial implementation, so it was impossible to figure out the value from a script, or correctly roundtrip the config file. Signed-off-by: Eli Schwartz --- src/pacman/pacman-conf.c | 13 + 1 file changed, 13 insertions(+) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 60739bf6..d76c0985 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -142,6 +142,14 @@ static void show_str(const char *directive, const char *val) printf("%s%c", val, sep); } +static void show_int(const char *directive, unsigned int val) +{ + if (verbose) { + printf("%s = ", directive); + } + printf("%u%c", val, sep); +} + static void show_list_str(const char *directive, alpm_list_t *list) { alpm_list_t *i; @@ -261,6 +269,8 @@ static void dump_config(void) show_bool("ILoveCandy", config->chomp); show_bool("NoProgressBar", config->noprogressbar); + show_int("ParallelDownloads", config->parallel_downloads); + show_cleanmethod("CleanMethod", config->cleanmethod); show_siglevel("SigLevel", config->siglevel, 0); @@ -372,6 +382,9 @@ static int list_directives(void) } else if(strcasecmp(i->data, "NoProgressBar") == 0) { show_bool("NoProgressBar", config->noprogressbar); + } else if(strcasecmp(i->data, "ParallelDownloads") == 0) { + show_int("ParallelDownloads", config->parallel_downloads); + } else if(strcasecmp(i->data, "CleanMethod") == 0) { show_cleanmethod("CleanMethod", config->cleanmethod); -- 2.26.2
Re: [pacman-dev] [PATCH] Make bash-completion optional
On 5/7/20 2:45 PM, Wouter Wijsman wrote: > The bash completion files were the only reason pacman was not able to > build as a non-root user. This patch adds the option to not install > these files. This was needed for my use case, hopefully upstream someone > else has a use for it as well. The problem should only happen during install, not during build. I presume that it is trying to install to e.g. /usr/share/bash-completion/completions/pacman and raising a PermissionError? To find the correct install dir for the completions, it first tries to look at pkg-config to see where pkg-config says they should be installed. It then falls back on --datadir and installs to ${datadir}/bash-completion/completions/ If you have bash-completion installed and its pkg-config file points at a root-owned location you'll need root to install pacman. If you don't have bash-completion installed, then your custom prefix might not actually be picked up by bash-completion. I wonder what the right solution is here. In autotools land, ./configure --help has this to say: Some influential environment variables: [...] bashcompdir value of completionsdir for bash-completion, overriding pkg-config This is part of the builtin functionality of the pkg.m4 macros which autotools is using. meson doesn't directly support anything like this. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH] doc: remove vim modelines from BUILDINFO(5)
We (thought we) removed all modelines from the project in commit 860e4c4943ad062bd0eff99f28e7d64804b3c08e, but apparently this one sneaked in by virtue of this manpage being added to the project after the "remove all the modelines" patch was submitted, but before it was applied. I must have failed to update the patch to remove it from this file also. Signed-off-by: Eli Schwartz --- Found this while pondering to write a pkg.tar(5) manpage. doc/BUILDINFO.5.asciidoc | 3 --- 1 file changed, 3 deletions(-) diff --git a/doc/BUILDINFO.5.asciidoc b/doc/BUILDINFO.5.asciidoc index bb300e18..72b297dd 100644 --- a/doc/BUILDINFO.5.asciidoc +++ b/doc/BUILDINFO.5.asciidoc @@ -1,6 +1,3 @@ -/ -vim:set ts=4 sw=4 syntax=asciidoc noet spell spelllang=en_us: -/ BUILDINFO(5) -- 2.26.2
Re: [pacman-dev] Setting HOME in build() breaks package signing
On 5/5/20 4:11 PM, brainpower wrote: [...] > This made me wonder: > Is this something makepkg should take care of (e.g.by restoring $HOME after > build() or ensuring gpg will use $OLDHOME/.gnupg) > or should such a PKGBUILD be considered broken / invalid? [...] > $ cat PKGBUILD > pkgname=broken-home > pkgver=1 > pkgrel=1 > arch=('any') > > build() { > export HOME="${srcdir}/tmphome" > } I would argue this should probably be a 'local' variable. e.g. build() { local HOME="${srcdir}/tmphome" some-command-that-needs-fakehome } Since HOME is previously marked as exportable, it is still getting exported, and modifying an exported variable causes the changes to be picked up. So this is identical: export somevar somevar=foo vs. export somevar=foo vs. somevar=foo export somevar And the 'local' attribute coexists with the exported attribute: $ foo() { declare -p somevar; local somevar=foo; export somevar; env | \ grep somevar; declare -p somevar; }; foo bash: declare: somevar: not found somevar=foo declare -x somevar="foo" $ declare -p somevar bash: declare: somevar: not found -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH 1/2] meson: use better check for debug builds
meson 0.48 added the 'debug' and 'optimization' builtin options, which bidirectionally map to the buildtype, but in some cases where debug is enabled, the builtype may be custom. Checking the 'debug' option lets us detect every case currently detected, plus a few more, and does so in a shorter and more concise manner. Signed-off-by: Eli Schwartz --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 572526b2..680cf62b 100644 --- a/meson.build +++ b/meson.build @@ -177,7 +177,7 @@ elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS') conf.set('FSSTATSTYPE', 'struct statfs') endif -if get_option('buildtype').startswith('debug') +if get_option('debug') extra_cflags = [ '-Wcast-align', '-Wclobbered', -- 2.26.2
[pacman-dev] [PATCH 2/2] editorconfig: set meson indentation style
meson.build gets two-space indents, but our global tabbed default was overriding this. Signed-off-by: Eli Schwartz --- .editorconfig | 4 1 file changed, 4 insertions(+) diff --git a/.editorconfig b/.editorconfig index efea87bf..220ffbc8 100644 --- a/.editorconfig +++ b/.editorconfig @@ -16,5 +16,9 @@ indent_style = tab [{NEWS,HACKING}] indent_style = space +[meson.build] +indent_style = space +indent_size = 2 + [*.py] indent_style = space -- 2.26.2
Re: [pacman-dev] Planning the move to the Arch Linux Gitlab instance
On 5/2/20 4:29 AM, Allan McRae wrote: > Hi all, > > Arch Linux is setting up its own Gitlab instance. I have been playing > around with it for the day and I think pacman should transition there. > > Note: the current pacman repo on the Arch Gitlab is a playground I have > been using. It will be deleted. > > There are some features that are of interest: > - CI on merge requests, which will catch build issues automatically. > - Review where I can easily see what changed on resubmission > - integrated bug tracker, patchwork, ... meaning I spend less time > updating things on multiple sites. > > We can also do things like provide regular developers a namespace. e.g. > I would own allan/* branches, and could prepare work and request merges > from branches there. > > > So how would we proceed? > > Bugs: > Do we pick a date where we disable new bug submissions on the current > tracker and point towards gitlab? Are old bug transferred after some > ruthless trimming - I don't think this can be automated... Do we leave > it open until all bugs get closed? > > Patchwork: > The got "cleared" somewhat recently, when an update occurred. At bit of > effort could followup al the changes needed that never were done and > clear this out. > > Mailing list: > New patches would no longer appear here. I don't know how much we can > get gitlab to report to the mailing list, or if we even want to. The > mailing list would still serve as a good place to discuss feature > implementation in a more public way. I know that you're supposed to be able to email patches to a gitlab address and have it generate a merge request. Not sure how notifications the other way work, if at all. I'd quite like to have, at a minimum, a post-receive notification to the mailing list like we currently have... If we don't allow absolutely anyone to create an account on our gitlab instance, then they cannot report bugs, or submit patches (even by email, it's a private email address tied to your account). IIRC devops were not eager to enable public account creation for various reasons. > Date: > What is a good time for all this to happen? Once Anatol's parallel > downloads patches are fully committed, I'd like to make a 6.0.0beta1 > release to get some good coverage on these changes. Could we move at > the same time? > > Let me have opinions. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] Bug: using --program-prefix configure option breaks makepkg
On 5/4/20 5:12 PM, wwijs...@live.nl wrote: > Thanks for the info! I did manage to fix it for autotools in the way > gcc does it, for which I've submitted a patch now, but I think you're > right about Meson. I don't really know much about Meson, though. Are > you guys planning on keeping autotools supported or is Meson the > future? In the long term, we'd like to declare meson feature-compatible and drop autotools. There is some information here: https://bugs.archlinux.org/task/64394 I don't know that meson would be adding such a feature, and this was never explicitly supported (rather, added for free by autotools), so I'm not sure whether to tell you to do manual moving around or whether to add an option for it and manually define executable names during configure time. >>> I compile with the following commands: ./configure -- >>> prefix=${PSPDEV} >>> --with-buildscript=PSPBUILD --with-root-dir=${PSPDEV}/psp >>> --program-prefix="psp-" --disable-doc make make install >>> >>> For this to work, I require the following patch: >>> https://github.com/sharkwouter/psptoolchain/blob/psp-pacman/patches/pacman-5.2.1.patch >>> >>> Is there a way to prevent me from needing this patch? Besides this >>> patch, the pacman 5.2.1 tarball build and works fine for my >>> purposes. >>> >>> P.S. I did find another small annoyance. Which is that "make >>> install" >>> fails if the bash completion files are already installed. >> >> Is this because of: >> >> for completion in makepkg pacman-key; do \ >> $(LN_S) pacman $(DESTDIR)/$(bashcompdir)/$$completion; \ >> done >> >> Because if so, you can probably get it to work by defining LN_S to >> add >> the -f flag. > > It seems this can be set in libtool.m4, but that's generated/added by > autoreconf it seems, so I don't know if that would work. I'll probably > add a workaround to my build script :/ If it works and covers your use case, then we can simply do this: $(LN_S) -f pacman $(DESTDIR)/$(bashcompdir)/$$completion -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] Showing optional dependencies in pactree output
On 5/4/20 1:28 PM, Frederick Zhang via pacman-dev wrote: > Hi all, > > I noticed that there is a patch by Johannes available at [1] which adds > a new flag to pactree to show optional dependencies as well. I think > this is a nice feature to have however it seems that the patch was never > merged. > > I just tried it out locally and managed to compile with a few minor > changes. If it wasn't rejected for some reason guess I could submit a > patch if needed? > > [1] https://lists.archlinux.org/pipermail/pacman-dev/2016-February/020922.html That sounds like a very interesting idea, and I'm not aware of it having been rejected for any reason. I'd guess it simply got lost. Please note, however, that in late 2016 pactree was removed from pacman, along with the other contrib scripts and programs, and refactored into a separate repo, "pacman-contrib". The dedicated [pacman-contrib] mailing list collects discussion about this, and would be an excellent place to submit a patch. :) -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] Bug: using --program-prefix configure option breaks makepkg
On 5/4/20 10:39 AM, Wouter Wijsman wrote: > Hi Pacman team, > > In the last couple of days I've been trying to build pacman for use > with a homebrew sdk for the Playstation portable. The idea is for it > to allow sharing of libraries for the system. To make sure this > version of pacman doesn't collide with the system's, I use the > --program-prefix option for the configure script, which adds "psp-" > in front of all binary names. > > Now when using the --program-prefix option, makepkg is no longer able > to work. This because it has the name for the pacman executable name > hardcoded it seems. I've never really experimented with --program-prefix, but I expect the general way to do this would be to add those executable names as replacements to build-aux/edit-script.sh.in (meson) and scripts/Makefile.am $(edit) (autotools). Is there e.g. a variable set in the configured Makefile which describes this prefix and can be used to define a replacement? Do you know of any examples of other autotools projects which invoke internal programs via shellscripts and have handled this before? I think for meson, this would need to be explicitly added as AFAIK meson doesn't have a comparable feature anyway. > I compile with the following commands: ./configure --prefix=${PSPDEV} > --with-buildscript=PSPBUILD --with-root-dir=${PSPDEV}/psp > --program-prefix="psp-" --disable-doc make make install > > For this to work, I require the following patch: > https://github.com/sharkwouter/psptoolchain/blob/psp-pacman/patches/pacman-5.2.1.patch > > Is there a way to prevent me from needing this patch? Besides this > patch, the pacman 5.2.1 tarball build and works fine for my > purposes. > > P.S. I did find another small annoyance. Which is that "make install" > fails if the bash completion files are already installed. Is this because of: for completion in makepkg pacman-key; do \ $(LN_S) pacman $(DESTDIR)/$(bashcompdir)/$$completion; \ done Because if so, you can probably get it to work by defining LN_S to add the -f flag. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] Planning the move to the Arch Linux Gitlab instance
On 5/4/20 2:20 AM, Anatol Pomozov wrote: > I've been using review tools like Gerrit and Gitlab at my dayjob for > almost 10 years now. And personally I find this way of reviewing > patches more productive than using maillist. Gitlab-like UI provides > reviewing context, adds syntax colorizing, allows to track what has > been reviewed and what has not. Plus Gitlab allows to automate some > boring tasks. patchwork also provides both syntax coloring and review tracking. Review context is provided for free. It's really not about these features. Gitlab is an integrated software forge which provides this all together, that's all. Better than some in some ways, worse than some in other ways. But if you value *integration*, then it's a win over having multiple locations to do everything. Is there a reason you sound like an infomercial? >> We can also do things like provide regular developers a namespace. e.g. >> I would own allan/* branches, and could prepare work and request merges >> from branches there. > > That would be great. I know Gerrit tool has $user namespaces where > people can keep large refactorings before it is ready for a review. I don't think anyone suggested moving to gerrit... developer namespaces are not a new concept anyway, you can do them with gitolite too. Gitlab may provide them as part of the package deal, sure, but we hardly need gerrit for this... > We should also invest some efforts into setting up Gitlab automation > using bots. Bots could run tests, check spelling, run lint, update > "pacman-git" AUR package etc, etc How are bots different from CI, which already runs tests? I'm not sure what you mean by "bots". (Note that Allan has submitted a patch to add .gitlab-ci.yml, before this thread even started, so as far as I can tell we don't need to invest any more efforts as it has already been done. If there are concerns about it, please address them there.) What "spelling checks" are you proposing, I don't think I've seen that before, plus spelling and grammar checkers are not exactly known for their sterling accuracy. Are there known problems with spelling in pacman? Prose is pretty unambiguously right or wrong, so if there are problems it should be a one-time fix. No one's updating the pacman-git PKGBUILD, the AUR guidelines forbid frivolous pkgver bumps on every upstream commit. When changes to e.g the build() function are needed, I'll be happy to update the PKGBUILD (since I am the current AUR maintainer), but it will be done by hand, and won't be because of gitlab... I already publish and sign binary packages in my custom repo for public use, and it's my belief that this stage should be manual anyway (I'm not giving my PGP signing key to a gitlab runner to use whenever it wants). >> So how would we proceed? >> >> Bugs: >> Do we pick a date where we disable new bug submissions on the current >> tracker and point towards gitlab? Are old bug transferred after some >> ruthless trimming - I don't think this can be automated... > > Do we want to try to move bugs to Gitlab or we start Gitlab bug > database from scratch? I personally fine with either case but if we > choose the latter option then bugs.archlinux.org should stay available > read-only as an archive of the discussions. > > Sven-Hendrik what is the current state of git/flyspray -> gitlab > migration toolset? I was under the impression previous flyspray work had been focused on bugzilla, not gitlab. Anyway, as far as I can tell Sven hasn't been CC'ed, nor is subscribed to this list, so I'm not sure who you're asking here. >> Do we leave >> it open until all bugs get closed? >> >> Patchwork: >> The got "cleared" somewhat recently, when an update occurred. At bit of >> effort could followup al the changes needed that never were done and >> clear this out. >> >> Mailing list: >> New patches would no longer appear here. I don't know how much we can >> get gitlab to report to the mailing list, or if we even want to. The >> mailing list would still serve as a good place to discuss feature >> implementation in a more public way. >> >> Date: >> What is a good time for all this to happen? Once Anatol's parallel >> downloads patches are fully committed, I'd like to make a 6.0.0beta1 >> release to get some good coverage on these changes. Could we move at >> the same time? > > I personally would love to try Gitlab for my "parallel-download" > patchset review. But I guess Gitlab instance is not ready for it > yet... Well, if it's not ready it's not ready... so let's just continue the in-progress review on the mailing list. It's not like there is an urgent need to see how it works for the sake of learning how it works. We
Re: [pacman-dev] [PATCH] Adds a "retry?" prompt for when transaction creation fails for user-friendlier fail support.
On 4/28/20 8:40 PM, Allan McRae wrote: > On 28/4/20 9:43 am, Pedro wrote: >> EDIT: Rewrote the prompt message and added a check for the --noconfirm flag >> >> >From what I could see, the function trans_init that comes right before the >> >prompt has already specific messages >> for each operation, so I consider that this new implementation fits the >> purpose. >> My motivation to submit this patch is that it's pretty common for >> AUR-helpers to automatically invoke pacman after a build, >> but correct transaction error handling is quite rare. So I thought that to >> implement a handling mechanism in the user side >> would solve the problem not only in that case, but also for other scripts >> that invoke pacman. >> >> Signed-off-by: Pedro >> --- > > I don't see how retrying the same operation with the same input is going > to help anyone. If pacman fails in these ways, it is not going to > succeed the next attempt. > > Allan Just like I said the first time this patch was submitted -- and now it just has a slightly longer "retry" message, but it still doesn't answer my question "what is the user supposed to do about it"? Pedro, a AUR helper is most likely going to fail because of dependency checks, which happen later. Do you have examples of a situation where pacman cannot even create the transaction, let alone populate it with packages? If so, why is it happening? These are questions that should be answered before adding a prompt with no clear purpose. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Adds a "retry?" prompt for when transaction creation fails for user-friendlier fail support.
What do you propose the user do about this if a transaction fails to be created? What should the user think if they see the message "Retry?" here? -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH v3] Split target output
} > + if (alpm_list_count(upgrade)) { > + pm_asprintf(_header, _("Upgrading (%lu):"), > alpm_list_count(upgrade)); > + list_display(upgrade_header, upgrade, cols); > + if (alpm_list_count(remove)) { > + printf("\n"); > + } > + } > + if (alpm_list_count(remove)) { > + pm_asprintf(_header, _("Removing (%lu):"), > alpm_list_count(remove)); > + list_display(remove_header, remove, cols); > + } > } > printf("\n"); > > table_free(header, rows); > - FREELIST(names); > - free(str); > + if (install) FREELIST(install); > + if (upgrade) FREELIST(upgrade); > + if (remove) FREELIST(remove); > + if (install_header) free(install_header); > + if (upgrade_header) free(upgrade_header); > + if (remove_header) free(remove_header); resolving dependencies... looking for conflicting packages... Upgrading (46): bind-tools-9.16.2-2 bluez-libs-5.54-2 checkbashisms-2.20.3-1 containerd-1.3.4-2 cryptsetup-2.3.1-2 dkms-2.8.1-2 fanficfare-3.18.0-1 firefox-developer-edition-76.0b8-1 gedit-3.36.2-1 gnome-terminal-3.36.2-1 gvim-8.2.0510-2 imagemagick-7.0.10.8-1 json-c-0.14-1 lib32-mesa-20.0.5-1 libmfx-20.1.1-1 libslirp-4.3.0-1 lilv-0.24.8-1 linux-firmware-20200421.78c0348-1 man-pages-5.06-2 meson-0.54.1-1 ndctl-68-2 perl-exporter-tiny-1.002002-1 python-distro-1.5.0-1 python-peewee-3.13.3-1 python-sphinx-3.0.3-1 python-wcwidth-0.1.9-1 python2-distro-1.5.0-1 python2-wcwidth-0.1.9-1 qbittorrent-4.2.5-1 rust-1:1.43.0-2 s-nail-14.9.19-1 serd-0.30.4-1 systemd-245.5-2 systemd-libs-245.5-2 systemd-sysvcompat-245.5-2 texinfo-6.7-3 tmux-3.1-1 tzdata-2020a-1 unzip-6.0-14 vim-runtime-8.2.0510-2 vte-common-0.60.2-1 vte3-0.60.2-1 webkit2gtk-2.28.2-1 wine-5.7-1 xapps-1.8.0-1 zip-3.0-9 Total Download Size: 88.22 MiB Total Installed Size: 1860.56 MiB Net Upgrade Size: 2.28 MiB double free or corruption (out) Aborted All I have are upgrades. > rows = NULL; > > if(dlsize > 0 || config->op_s_downloadonly) { > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] (no subject)
On 4/17/20 6:22 PM, Carson Black wrote: > This asciicast demonstrates the output of this patch: > https://asciinema.org/a/321160 Can you use LC_ALL=C for stuff like this? For much the same reason the patch commit message itself is in English -- if this context helps us see live what the patch does, and we want to understand it, English is the generally accepted way to do it. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Touch up proto files
On 4/12/20 5:01 PM, Daniel M. Capella wrote: > - split: Fix erroneous $pkgname calls to use $pkgbase It's not automatically erroneous to use $pkgname in a split package, ${pkgname[0]} is commonly the same value as $pkgbase and there's no problem using either one. You might need to be more specific for package_*() alone... but those invocations actually use pkgbase If this is about making the current inconsistent use of both pkgbase and pkgname all over the file, then can the commit message mention this? At the moment it is certainly no *clearer* to the user which one they might want to use, and why one might recommend avoiding the use of $pkgname... > - VCS: > > - Add missing fields What is missing about them? These proto files don't need to be a complete command reference for makepkg variables, the authoritative documentation which people actually use is PKGBUILD(5). I'd even rather we remove the unused ones entirely to match how we expect users to write production PKGBUILDs. :p At least -vcs and -split probably don't need to repeat everything which PKGBUILD.proto has prototyped, they only really need to prototype how to adapt a git source or split packaging function. > - Remove unnecessary $srcdir's This is a stylistic choice, and cd'ing to absolute paths rooted in $srcdir is useful if you do something like cd "${srcdir}"/foo do_something cd "${srcdir}"/bar do_something since cd ../bar can be difficult to follow the more times you do it. Personally, I always use $srcdir in my cd's. > - Move ./autogen.sh to prepare() Running ./autogen.sh is actually one of my pet peeves, it is flatly incorrect. GNU autotools documentation considers this to be deprecated and you should use -- and ensure the build system uses -- autoreconf when- and where- ever possible (which is nearly always, unless configure.ac is missing some builtin macro, and that should be fixed). While I agree that autogen.sh should not be run in PKGBUILD-vcs.proto's build() function, I would change it to autoreconf -fi at the same time as moving it to prepare. > - 80 char line length for comments and make license note stand out The current comments are already 80 lines, so I'm not sure what this means? > - Add ./autogen.sh to the other proto's Continuing on from the previous point about autotools autogen/autoreconf usage... For dist tarballs uploaded to ftp sites, autoreconf will have already been run, there is no need to rerun it except for VCS packages, which is why PKGBUILD.proto and PKGBUILD-split.proto don't have it at all. I do not believe we should add this as it violates the expected use of autotools for stable release tarballs. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] Fw: Pacman support for IPFS
On 4/12/20 9:19 PM, @RubenKelevra wrote: > Sorry, the mail slipped by - or I had answered sooner. > > Yes, that's possible. The url is indeed ipfs://$CID > > If you got the browser-plugin installed for IPFS, those links will be > automatically converted to something browser can understand, like > http://127.0.0.1:8080/ipfs/$CID which then requests the file from the > local http-gateway of the ipfs-node. > > If you have Opera for Android (which got build in IPFS-Support) those > ipfs://$CID links will natively work. > > Maybe we could define something like the DLAGENTS in PKGBUILDS, this > would allow to extend the field in the future: > > ipfs::ipfs://$CID As far as pacman is concerned, *all* urls will be directly downloaded with curl. And curl in turn supports many other protocols, though not AFAICT ipfs. I'd probably recommend running a local http->ipfs proxy for this. I guess you could also use a custom XferCommand which only supports ipfs, once you switch your mirrorlist to only use an ipfs server? How is this content id generated? Is it deterministically generated based on the file contents, so that repo-add can generate it the same way it generates the checksum hash, and do so *offline*? Or can different people end up with different content ids when uploading the same file? -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Use noextract with pacman-conf NoExtract
On 4/13/20 3:02 AM, Allan McRae wrote: > On 2/4/20 1:29 am, Eli Schwartz wrote: >> From: Earnestly >> >> Current code accidently uses noupgrade for the NoExtract directive. >> >> https://medium.com/@Code_Analysis/the-last-line-effect-7b1cb7f0d2a1 > > Ack. But we don't need links to irrelevant websites in the commit > message... I've directly reposted the patch on Earnestly's behalf, and that is where the website link came from. Personally, I'm fine with simply dropping that line. Feel free to do so when pulling the patch. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
[pacman-dev] [PATCH] Use noextract with pacman-conf NoExtract
From: Earnestly Current code accidently uses noupgrade for the NoExtract directive. https://medium.com/@Code_Analysis/the-last-line-effect-7b1cb7f0d2a1 Signed-off-by: Eli Schwartz --- Previously fixed in https://github.com/andrewgregory/pacutils/commit/64c7d0aca1a3aa3be5b7789f10ee1e5c9e0d7471 but the unfixed version got merged here. :/ src/pacman/pacman-conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 939cbdbd..f67ec21f 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -348,7 +348,7 @@ static int list_directives(void) } else if(strcasecmp(i->data, "NoUpgrade") == 0) { show_list_str("NoUpgrade", config->noupgrade); } else if(strcasecmp(i->data, "NoExtract") == 0) { - show_list_str("NoExtract", config->noupgrade); + show_list_str("NoExtract", config->noextract); } else if(strcasecmp(i->data, "Architecture") == 0) { -- 2.26.0
Re: [pacman-dev] [PATCH] Use C18 language standard
On 3/31/20 4:12 PM, Anatol Pomozov wrote: > C18 is the latest stable specification of the language that is > supported by modern toolchains. > It sounds reasonable to me if pacman keeps up with the technology > stack and uses its advancements. What advancements are used as a direct result of passing -std=c11? What generated object code does this patch *on its own*, change? > Are you saying that C18 requirement introduces limits due to > requirement of this 2 years old toolchains? Just curious what > platforms where pacman is used lacks these toolchains? The limit it introduces is "over-specifying requirements". > It is not really required to use all the spec features right away. But > having an option to use it in the future is gonna be useful. > > As of particular use case - if in the future we decide to speed-up the > installation with more thread-level parallelism (e.g. checking > signatures/unpacking/... on multiple CPUs) then better C11 > mutlithreadding support is very useful. If we need it in the future, we can add it in the future. Am I missing something? If I add a new pacman feature that relies on libfoo.so, I add a dependency on libfoo.so -- but does that mean I submit a patch the year before, to "pass -lfoo when building pacman, to make the libfoo library an option since it will be useful in the future"? No, I submit a patch that introduces the use of libfoo functions, and in the same patch, I add -lfoo to the build flags. So, it seems reasonable to wait until we want to add c11 features to pacman, and then submit a patch with the new features, and bump the minimum-required-language-version argument at the same time. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] meson: use 'pedantic' compiler warning level
On 3/26/20 4:42 PM, Eli Schwartz wrote: > On 3/26/20 3:32 PM, Anatol Pomozov wrote: >> Hello >> >> In this case I would like to propose to move forward with my original >> patch and enable 'pedantic' warning level by default. It will match >> the flag with the Makefile based build. > The original patch did not clarify this was about parity with autotools. > I've looked around a bit and my conclusions are as follows: > > - We always defaulted to -pedantic -D_GNU_SOURCE -Wall in autotools, > plus additional flags if --enable-warningflags > > - meson will, with warning_level 3, add -Wall -Wextra -Wpedantic, which > means this patch would add the heretofore-unseen -Wextra > (warning_level 1 only adds -Wall, 2 adds -Wall -Wextra, no option is > available for those who like -Wpedantic but don't want -Wextra) > > Since autotools unconditionally added these flags, parity suggests > always adding them in meson too. Do we care about -Wextra? We currently > build with -Wextra without warnings being emitted, anyway. Allan confirmed on IRC that we do care, and this patch will not be accepted because we do not want -Wextra. > If -Wextra is fine then we can just take this current patch, but amend > it to add -Wextra to configure.ac near > > ``` > if test "x$debug" = "xyes" ; then > ``` > > Anatol, can you also update the commit message to mention that this is > implementing parity with the default autotools warning level? > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] meson: use 'pedantic' compiler warning level
On 3/26/20 3:32 PM, Anatol Pomozov wrote: > Hello > > In this case I would like to propose to move forward with my original > patch and enable 'pedantic' warning level by default. It will match > the flag with the Makefile based build. The original patch did not clarify this was about parity with autotools. I've looked around a bit and my conclusions are as follows: - We always defaulted to -pedantic -D_GNU_SOURCE -Wall in autotools, plus additional flags if --enable-warningflags - meson will, with warning_level 3, add -Wall -Wextra -Wpedantic, which means this patch would add the heretofore-unseen -Wextra (warning_level 1 only adds -Wall, 2 adds -Wall -Wextra, no option is available for those who like -Wpedantic but don't want -Wextra) Since autotools unconditionally added these flags, parity suggests always adding them in meson too. Do we care about -Wextra? We currently build with -Wextra without warnings being emitted, anyway. If -Wextra is fine then we can just take this current patch, but amend it to add -Wextra to configure.ac near ``` if test "x$debug" = "xyes" ; then ``` Anatol, can you also update the commit message to mention that this is implementing parity with the default autotools warning level? -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] Translation issues
On 3/17/20 9:53 AM, Luca Weiss wrote: > On March 17, 2020 5:17:18 AM GMT+01:00, Allan McRae > wrote: >> Looks like this string was fixed a month ago too. > > Great; but that fix is definitely not pulled into git yet as of today ;) Transifex updates are only checked into git right before a release, to avoid senseless code churn. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] makepkg: drop duplicate reporting of missing dependencies
On 3/17/20 12:09 AM, Allan McRae wrote: > On 6/2/20 10:48 pm, Dave Reisner wrote: >> When pacman fails to satisfy deps, we might see output like the >> following: >> >> ==> Making package: spiderfoot 3.0-1 (Thu 06 Feb 2020 12:45:10 PM CET) >> ==> Checking runtime dependencies... >> ==> Installing missing dependencies... >> error: target not found: python-pygexf >> ==> ERROR: 'pacman' failed to install missing dependencies. >> ==> Missing dependencies: >> -> python-dnspython >> -> python-exifread >> -> python-cherrypy >> -> python-beautifulsoup4 >> -> python-netaddr >> -> python-pysocks >> -> python-ipwhois >> -> python-ipaddress >> -> python-phonenumbers >> -> python-pypdf2 >> -> python-stem >> -> python-whois >> -> python-future >> -> python-pyopenssl >> -> python-docx >> -> python-pptx >> -> python-networkx >> -> python-cryptography >> -> python-secure >> -> python-pygexf >> -> python-adblockparser >> ==> Checking buildtime dependencies... >> ==> ERROR: Could not resolve all dependencies. >> >> This is misleading -- the only truly missing package is python-pygexf, >> but we fail to remove sync-able deps from our deplist and report >> everything as if it were missing. Simply drop this extra reporting >> because pacman already tells us exactly what couldn't be resolved. >> --- > > It seems this went too far: > > Before: > > $ makepkg > ==> Making package: pacman 5.2.1-4 (Tue 17 Mar 2020 14:05:51) > ==> Checking runtime dependencies... > ==> Missing dependencies: > -> pacman-mirrorlist > ==> Checking buildtime dependencies... > ==> ERROR: Could not resolve all dependencies. > > After: > > $ makepkg > ==> Making package: pacman 5.2.1-4 (Tue 17 Mar 2020 14:01:13) > ==> Checking runtime dependencies... > ==> Checking buildtime dependencies... > ==> ERROR: Could not resolve all dependencies. > > I have reverted on my patchqueue branch for the time being, but won't > push to master. I need more time to look at this... > > Allan Hrm, the idea here was to rely on handle_deps running pacman -S and returning pacman's own error messages. But if -s is not invoked, then we do still need to print it ourselves. Actually, thinking about this I'm not even sure I understand the patch in the first place. We're dealing with two errors here: one from pacman, saying it could not find the sync'able dependency, and one from makepkg, saying that due to pacman aborting, all of these packages are missing. Where it might make a difference is if the user reads the error message from makepkg -s: ==> Installing missing dependencies... error: target not found: foo ==> ERROR: 'pacman' failed to install missing dependencies. ==> Missing dependencies: -> bar -> foo And following that logic, goes and installs pacman -U ./something-which provides foo, plus pacman -S bar, and then runs makepkg without -s With this commit, they only install foo, then makepkg fails again due to bar being missing. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Provides generation for files (a.k.a. rpm fileattrs for makepkg)
On 3/8/20 6:49 PM, Eli Schwartz wrote: > On 3/8/20 6:39 PM, Carson Black wrote: >>> What is "cleaned", and how is the xargs program "cleaning" it? >> >> Excess whitespace is being trimmed using xargs. " a string >> " will get normalized to "a string" when passed through xargs. > > Examples of desktop files with this issue? I didn't find one like it on > my system. > >>> >>>> + >>>> + [[ -z "$cleaned" ]] && continue >>> >>> When will this be set but empty? >> >> Trailing semicolon in desktop file will result in a blank entry in the >> mimetypes array, >> resulting in a blank can-open-mimetype() provide without this code. > > $ mapfile -t -d ";" mimetypes < <(grep MimeType= > /usr/share/applications/qemu.desktop | cut -d '=' -f 2) > $ declare -p mimetypes > declare -a mimetypes=() Not sure what I was thinking here, lol, it is in fact a problem when the Mimetypes= line *does* exist. A good way to solve this would be [[ -n ${mimetypes[-1]//$'\n'} ]] || unset mimetypes[-1] -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Provides generation for files (a.k.a. rpm fileattrs for makepkg)
On 3/9/20 2:16 AM, Erich Eckner wrote: > Hi, > > On Mon, 9 Mar 2020, Allan McRae wrote: > >> People have previously suggested automatically adding all dependencies >> on libraries to depends, and have all packages provide their libraries. >> This was rejected. > > I had requested something similar once, but not exactly what you wrote > (I assume, you refer to my request - if not, please ignore): > > The request was to add all the provides parts, but the depends part only > if one likes to (e.g. if one evaluates the stability of linking more > important than an easy build path for upgrades) - but even that got > rejected. > > So in essence: Already *adding* the metadata is not accepted. Even if > it's not intended to use it thoroughly in packages. Adding provides=(libfoo.so) to all Arch PKGBUILDs wouldn't be a pacman-dev change. IIRC people have actually asked that makepkg automatically search through the $pkgdir and add provides=(libfoo.so=1-64) for every single installed library, whether the PKGBUILD instructs to do so or not. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Provides generation for files (a.k.a. rpm fileattrs for makepkg)
On 3/8/20 9:45 PM, Allan McRae wrote: > On 9/3/20 6:04 am, Carson Black wrote: >> 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) >> >> 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. > > Lets take a step back from even looking at the patch and whether it > works, and focus on whether this should be a feature of makepkg/pacman > at all. > > Firstly, I am very against makepkg performing action that are not > obvious from reading a PKGBUILD. This is why our templating system > pulls the code into the PKGBUILD instead of just providing a library of > functions to use. That way we can read PKGBUILDs to see the > build/package commands, unlike ebuilds. It is also why libprovides & > libdepends require specifying the library name, and only the version is > automatically added. Yep -- like I mentioned before, this should be opt-in via something explicitly in the PKGBUILD, libprovides is a good example of how to do that. > People have previously suggested automatically adding all dependencies > on libraries to depends, and have all packages provide their libraries. > This was rejected. > > So, I can't see me accepting this idea either. > > > However, we have started splitting makepkg into smaller parts, and > allowing some parts to be extendable with drop-in files. I am using > that to do checking on PKGBUILDs and packages from within makepkg > (https://github.com/allanmcrae/pkglint), and people have used it to add > support for more build options (e.g. the following packages in the AUR: > makepkg-optimize, makepkg-tidy-ect, makepkg-tidy-pdfsizeopt, > makepkg-tidy-scripts-git) > > One idea could be to add a method for extending a packages metadata > while writing .PKGINFO. I would not accept any functions adding > metadata into the pacman code base (except maybe some examples, not > enabled by default), but this serves as a point for distributions to > make their choices. I kind of also need to extend this for my WIP / languishing attempts at https://git.archlinux.org/users/eschwartz/pacman.git/log/?h=autoversioned-dependencies, so there's definitely some interesting stuff you could do with this. That being said, changes to the metadata generation have a tendency to become thing that also need lint_pkgbuild changes, so I'm not 100% convinced thirdparty extensions can be done here *safely*. (I suppose we could just say "eh, if you modify it, it's your fault if it breaks".) -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Introduce alpm_dbs_update() function for parallel db updates
On 3/8/20 4:55 PM, Anatol Pomozov wrote: >> Note that once review is complete, this patch (and the config one) will >> sit on a branch until the rest of the work is ready to land in one go. > > It is fine. But please share this branch with me so I can rebase my > work on top of the reviewed/accepted changes. I do not want to keep > more than a couple of changes in my development branch. Once review > for previous changes is done I'll start working on the new changes. You should in the future be able to find the branch in the same place Allan publishes other WIP feature branches, which, for future notice, is here: https://git.archlinux.org/users/allan/pacman.git/refs/ -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Provides generation for files (a.k.a. rpm fileattrs for makepkg)
On 3/8/20 6:54 PM, Carson Black wrote: >>>> Did you actually try this? >>> >>> Yes, I've been using the following packages as tests for this and >>> inspecting their metadata by hand: >>> >>> - vte3 (pkgconfig) >>> - kwayland (cmake) >>> - discover (desktop file) >>> >>> I know pacman can already install packages given a provides capability, >>> so I didn't bother setting up a local test repository to confirm the >>> fact that pacman >>> can install these packages given their capabilities. >> >> You're not reading what I said. > > Pardon, what did you mean by "Did you actually try this"? > > The large quote of the patch's description immediately beforehand had > me assuming that you were asking about > if I had tested the patch. I meant "look at the next few paragraphs, where I will explain why I don't think you tested this". And I explicitly ended off with "one would still NOT be able to use the automatically generated provides for packaging". But you know what, given you've said > Yes, I've been using the following packages as tests for this and > inspecting their metadata by hand: > I know pacman can already install packages given a provides capability, > so I didn't bother setting up a local test repository to confirm the > fact that pacman > can install these packages given their capabilities. I guess instead of not trying "use this as a packaging dependency in PKGBUILDs", you simply didn't test it at all, which is worse than I initially thought. So in the interest of proving that "it got successfully written to the .PKGINFO file" is not sufficient proof that the entire toolchain works, I invite you to add this repo to pacman.conf (I hacked up write_pkginfo to verbatim write the contents of any PKGBUILD-supplied function by a given name, PKGBUILDs are in this http directory): [junk] Server = https://pkgbuild.com/~eschwartz/repo/junk/ And run: $ printf '%s\n' '-`.^&=>=<: 1.0.0.0-:0 $""濫' | sudo pacman -Syu - $ printf '%s\n' '-`.^&=>=<: 1.0.0.0-:0/ $""濫' | sudo pacman -Syu - It's provided here, so it should work, right? : $ bsdtar -xOf test-junk2-1-1-any.pkg.tar.zst .PKGINFO | grep provides provides = -`.^&=>=<: 1.0.0.0-:0 $""濫 $ bsdtar -xOf test-junk-1-1-any.pkg.tar.zst .PKGINFO | grep provides provides = -`.^&=>=<: 1.0.0.0-:0/ $""濫 ... Conclusion: just because it is in the .PKGINFO, doesn't mean pacman itself can read it. You need to actually look at the changes you make, and verify the parser will accept it, and document that it works. P.S. pacman *can* read 'pkgconfig(libcurl)' provided by the same database, so that at least will be accepted by *pacman*'s parser. Even though you didn't test it. Even though makepkg won't let you depend on it in another package. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Provides generation for files (a.k.a. rpm fileattrs for makepkg)
On 3/8/20 6:39 PM, Carson Black wrote: >> What is "cleaned", and how is the xargs program "cleaning" it? > > Excess whitespace is being trimmed using xargs. " a string > " will get normalized to "a string" when passed through xargs. Examples of desktop files with this issue? I didn't find one like it on my system. >> >>> + >>> + [[ -z "$cleaned" ]] && continue >> >> When will this be set but empty? > > Trailing semicolon in desktop file will result in a blank entry in the > mimetypes array, > resulting in a blank can-open-mimetype() provide without this code. $ mapfile -t -d ";" mimetypes < <(grep MimeType= /usr/share/applications/qemu.desktop | cut -d '=' -f 2) $ declare -p mimetypes declare -a mimetypes=() You're wrong. >> Did you actually try this? > > Yes, I've been using the following packages as tests for this and > inspecting their metadata by hand: > > - vte3 (pkgconfig) > - kwayland (cmake) > - discover (desktop file) > > I know pacman can already install packages given a provides capability, > so I didn't bother setting up a local test repository to confirm the > fact that pacman > can install these packages given their capabilities. You're not reading what I said. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Provides generation for files (a.k.a. rpm fileattrs for makepkg)
esac > + ;; > + esac > +} > + > +desktop_prov() { > + case "$1" in > + *.desktop) > + grep -q "Type=Application" "$1" || return > + grep -q "Exec=" "$1" || return Why do either of these conditions matter if the file provides a mimetype? Surely that check alone is sufficient? > + base="$(basename $1)" > + echo "app(${base%.desktop})" > + > + mapfile -t -d ";" mimetypes < <(grep MimeType= $1 | cut > -d '=' -f 2) > + for mimetype in "${mimetypes[@]}"; do > + cleaned=$(echo $mimetype | xargs) What is "cleaned", and how is the xargs program "cleaning" it? > + > + [[ -z "$cleaned" ]] && continue When will this be set but empty? > + echo "can-open-mimetype($cleaned)" > + done > + ;; > + esac > +} > + > +providers=( > + pkgconfig cmake desktop > +) > + > +find_fileattrs_provides() { > + local files > + > + mapfile -t files < <(find "$pkgdir" -type f) Instead of storing it in a "files" variable first, I'd actually recommend using a while loop... > + > + for file in "${files[@]}"; do > + for function in "${providers[@]}"; do > + "$function"_prov "$file" > + done > + done > +} > > find_libprovides() { > local p libprovides missing > @@ -612,12 +677,14 @@ write_pkginfo() { > > mapfile -t provides < <(find_libprovides) > mapfile -t depends < <(find_libdepends) > + (( "$GENPROVIDES" != 0 )) && mapfile -t generated_provides < > <(find_fileattrs_provides) > > write_kv_pair "license" "${license[@]}" > write_kv_pair "replaces""${replaces[@]}" > write_kv_pair "group" "${groups[@]}" > write_kv_pair "conflict""${conflicts[@]}" > write_kv_pair "provides""${provides[@]}" > + write_kv_pair "provides""${generated_provides[@]}" > write_kv_pair "backup" "${backup[@]}" > write_kv_pair "depend" "${depends[@]}" > write_kv_pair "optdepend" "${optdepends[@]//+([[:space:]])/ }" > @@ -980,6 +1047,7 @@ usage() { > printf -- "$(gettext " --nocheckDo not run the %s function in > the %s")\n" "check()" "$BUILDSCRIPT" > printf -- "$(gettext " --noprepare Do not run the %s function in > the %s")\n" "prepare()" "$BUILDSCRIPT" > printf -- "$(gettext " --nosign Do not create a signature for > the package")\n" > + printf -- "$(gettext " --noprovidesgen Do not autogenerate > provides")\n" > printf -- "$(gettext " --packagelistOnly list package filepaths > that would be produced")\n" > printf -- "$(gettext " --printsrcinfo Print the generated SRCINFO > and exit")\n" > printf -- "$(gettext " --sign Sign the resulting package > with %s")\n" "gpg" > @@ -1029,7 +1097,7 @@ OPT_LONG=('allsource' 'check' 'clean' 'cleanbuild' > 'config:' 'force' 'geninteg' >'help' 'holdver' 'ignorearch' 'install' 'key:' 'log' 'noarchive' > 'nobuild' >'nocolor' 'nocheck' 'nodeps' 'noextract' 'noprepare' 'nosign' > 'packagelist' >'printsrcinfo' 'repackage' 'rmdeps' 'sign' 'skipchecksums' > 'skipinteg' > - 'skippgpcheck' 'source' 'syncdeps' 'verifysource' 'version') > + 'skippgpcheck' 'source' 'syncdeps' 'verifysource' 'version' > 'noprovidesgen') > > # Pacman Options > OPT_LONG+=('asdeps' 'noconfirm' 'needed' 'noprogressbar') > @@ -1070,6 +1138,7 @@ while true; do > --nocheck)RUN_CHECK='n' ;; > --noprepare) RUN_PREPARE='n' ;; > --nosign) SIGNPKG='n' ;; > + --noprovidesgen) GENPROVIDES=0 ;; This is unreproducible, the public metadata of the package now depends on command-line options. Also, no official repository packages would be built with manual command-line options, so you'd never be able to use the results. How does this help you if it only applies to packages you personally build? > -o|--nobuild) BUILDPKG=0 NOBUILD=1 ;; > -p) shift; BUILDFILE=$1 ;; > --packagelist)BUILDPKG=0 PACKAGELIST=1 IGNOREARCH=1;; > -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH] Provides generation for files (a.k.a. rpm fileattrs for makepkg)
On 3/8/20 4:04 PM, Carson Black wrote: > Your feedback on the code should have been addressed. > >> 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. > > I really don't consider this "too many ways to depend on the exact same > thing." The entire point is that you're describing what capabilities you > want from a package instead of naming the package explicitly. But that is in fact the exact same thing. You're describing "why" you want the package, not "what" you want. This is the problem that I have, because there's an infinite number of "why"s possible. An rpm can do something like depends=("/usr/bin/sh") which seems pretty silly to me! Where does it end? > And yes, I'm aware of the ability to query packages providing files, > but looking through filenames is not as efficient a workflow as throwing a > provides at pacman and letting it find a package that provides it. pkgfile -q KF5whatever.cmake | pacman -S - Seems more or less "efficient" to me, but I'm not even sure why that's a goal. package dependencies are created once, so the focus should be less on how *fast* you can pacman -S 'the-dependency' and more on the technical merits of conveying the information that way. And I'm unconvinced that automatically generating "reasons" to install a package is the right way to go here. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature