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&ls 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] [GIT] The official pacman repository branch, master, updated. v6.0.0alpha1-26-ga023565e
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 branch, master has been updated via a023565ed370851fd5bf61298460fe0adb0b4189 (commit) via ccdd1e3fd92591755e2b94bf63416c7b30cd217a (commit) via 831fc568fc87a75bb6e05575b93a7541b49e7aba (commit) from 95ffdd68b250af1d37067fe6dd70fc6a6094bc62 (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log - commit a023565ed370851fd5bf61298460fe0adb0b4189 Author: Eli Schwartz Date: Mon Dec 28 21:36:35 2020 -0500 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 Signed-off-by: Allan McRae commit ccdd1e3fd92591755e2b94bf63416c7b30cd217a Author: Emil Velikov Date: Wed Dec 23 22:43:57 2020 + Move hex_representation() to src/common We'll reuse the function in pacman with a later commit. Signed-off-by: Emil Velikov Signed-off-by: Allan McRae commit 831fc568fc87a75bb6e05575b93a7541b49e7aba Author: Emil Velikov Date: Wed Dec 23 22:43:56 2020 + Remove pre libarchive 3.0 code Pacman has required libarchive 3.0 or later for quite some time mow. Signed-off-by: Emil Velikov Signed-off-by: Allan McRae --- Summary of changes: doc/Doxyfile.in | 4 ++-- doc/meson.build | 1 + lib/libalpm/libarchive-compat.h | 20 lib/libalpm/util.c | 24 src/common/util-common.c| 26 ++ src/common/util-common.h| 1 + 6 files changed, 30 insertions(+), 46 deletions(-) hooks/post-receive -- The official pacman repository
Re: [pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
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&ls 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... Allan
Re: [pacman-dev] [PATCH 3/3] pacman: add file checksum validation against mtree
On 24/12/20 8:43 am, Emil Velikov wrote: > With libarchive v3.5.0 we have API to fetch the digest from the mtree. > Use that to validate if the installed files are modified or not. > > As always, a modified backup file will trigger a warning but will not > result in an actual failure. > > TODO: localization... no idea how that is even done :-) Adding the _() around the strings is all that needed done. > NOTE: indentation is likely all over the place - first time I see ts=2 For line wraps, we generally just use two indents, so really does not matter what the tab system is. > Signed-off-by: Emil Velikov > --- > src/pacman/check.c | 66 +- > 1 file changed, 59 insertions(+), 7 deletions(-) > > diff --git a/src/pacman/check.c b/src/pacman/check.c > index 02217d0f..083c547d 100644 > --- a/src/pacman/check.c > +++ b/src/pacman/check.c > @@ -176,19 +176,70 @@ static int check_file_size(const char *pkgname, const > char *filepath, > return 0; > } > > -/* placeholders - libarchive currently does not read checksums from mtree > files > -static int check_file_md5sum(const char *pkgname, const char *filepath, > - struct stat *st, struct archive_entry *entry, int backup) > +#if ARCHIVE_VERSION_NUMBER >= 3005000 This does not need wrapped in #if. There is nothing libarchive specific in this function. > +static int check_file_cksum(const char *pkgname, const char *filepath, > + int backup, const char *cksum_name, const char *cksum_calc, > const char *cksum_mtree) > { > + if(!cksum_calc || !cksum_mtree) { Only one of these failures matches the error message. Split into "checkusm information not available" and "failed to calculate checksum" > + if(!config->quiet) { > + pm_printf(ALPM_LOG_WARNING, _("%s: %s (failed to > compute %s checksum)\n"), > + pkgname, filepath, cksum_name); > + } > + return 1; > + } > + > + if(strcmp(cksum_calc, cksum_mtree) != 0) { > + if(backup) { > + if(!config->quiet) { > + printf("%s%s%s: ", config->colstr.title, > _("backup file"), > + config->colstr.nocolor); > + printf(_("%s: %s (%s checksum mismatch)\n"), > + pkgname, filepath, cksum_name); > + } > + return 0; > + } > + if(!config->quiet) { > + pm_printf(ALPM_LOG_WARNING, _("%s: %s (%s checksum > mismatch)\n"), > + pkgname, filepath, cksum_name); > + } > + return 1; OK. > + } > + > return 0; > } > +#endif > + > +static int check_file_md5sum(const char *pkgname, const char *filepath, > + struct archive_entry *entry, int backup) > +{ > + int errors = 0; > +#if ARCHIVE_VERSION_NUMBER >= 3005000 > + char *cksum_calc = alpm_compute_md5sum(filepath); > + char *cksum_mtree = hex_representation(archive_entry_digest(entry, > + > ARCHIVE_ENTRY_DIGEST_MD5), 16); > + errors = check_file_cksum(pkgname, filepath, backup, "MD5", cksum_calc, > + > cksum_mtree); > + free(cksum_mtree); > + free(cksum_calc); > +#endif > + return (errors != 0 ? 1 : 0); > +} > > static int check_file_sha256sum(const char *pkgname, const char *filepath, > - struct stat *st, struct archive_entry *entry, int backup) > + struct archive_entry *entry, int backup) > { > - return 0; > + int errors = 0; > +#if ARCHIVE_VERSION_NUMBER >= 3005000 > + char *cksum_calc = alpm_compute_sha256sum(filepath); > + char *cksum_mtree = hex_representation(archive_entry_digest(entry, > + > ARCHIVE_ENTRY_DIGEST_SHA256), 32); > + errors = check_file_cksum(pkgname, filepath, backup, "SHA256", > cksum_calc, > + > cksum_mtree); > + free(cksum_mtree); > + free(cksum_calc); > +#endif > + return (errors != 0 ? 1 : 0); > } > -*/ > > /* Loop through the files of the package to check if they exist. */ > int check_pkg_fast(alpm_pkg_t *pkg) > @@ -369,7 +420,8 @@ int check_pkg_full(alpm_pkg_t *pkg) > > if(type == AE_IFREG) { > file_errors += check_file_size(pkgname, filepath, &st, > entry, backup); > - /* file_errors += check_file_md5sum(pkgname, filepath, > &st, entry, backup); */ > + file_errors += check_file_md5sum(pkgname, filepath, > entry, backup); > +
Re: [pacman-dev] [PATCH 1/3] Remove pre libarchive 3.0 code
On 24/12/20 8:43 am, Emil Velikov wrote: > Pacman has required libarchive 3.0 or later for quite some time mow. > > Signed-off-by: Emil Velikov > --- > lib/libalpm/libarchive-compat.h | 20 > 1 file changed, 20 deletions(-) > Looks good.
[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