[pacman-dev] [PATCH 1/4] pacman/conf: Remove unused include
--- src/pacman/conf.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 9a37dcea..29f69052 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -35,7 +35,6 @@ #include "conf.h" #include "ini.h" #include "util.h" -#include "pacman.h" #include "callback.h" /* global config variable */ -- 2.18.0
This is shared between pacman and pacman-conf (and might be used by other binaries in the future) -- no need to compile it once for each consumer. --- src/pacman/.gitignore | 3 ++- src/pacman/Makefile.am | 37 +++++++++++++++++++------------------ 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/pacman/.gitignore b/src/pacman/.gitignore index 24e11ed3..9889c35e 100644 --- a/src/pacman/.gitignore +++ b/src/pacman/.gitignore @@ -1,6 +1,7 @@ .deps .libs +*.l[ao] pacman pacman.exe pacman-conf -pacman-conf.exe \ No newline at end of file +pacman-conf.exe diff --git a/src/pacman/Makefile.am b/src/pacman/Makefile.am index 2344daff..15cf20ce 100644 --- a/src/pacman/Makefile.am +++ b/src/pacman/Makefile.am @@ -8,6 +8,19 @@ hookdir = ${sysconfdir}/pacman.d/hooks/ cachedir = ${localstatedir}/cache/pacman/pkg/ logfile = ${localstatedir}/log/pacman.log +noinst_LTLIBRARIES = \ + libbasic.la + +libbasic_la_SOURCES = \ + conf.h conf.c \ + ini.h ini.c \ + callback.h callback.c \ + util.h util.c \ + util-common.h util-common.c + +libbasic_la_LIBADD = \ + $(top_builddir)/lib/libalpm/.libs/libalpm.la + bin_PROGRAMS = pacman pacman-conf AM_CPPFLAGS = \ @@ -31,37 +44,25 @@ endif pacman_SOURCES = \ check.h check.c \ - conf.h conf.c \ database.c \ deptest.c \ files.c \ - ini.h ini.c \ package.h package.c \ pacman.h pacman.c \ query.c \ remove.c \ sighandler.h sighandler.c \ sync.c \ - callback.h callback.c \ - upgrade.c \ - util.h util.c \ - util-common.h util-common.c + upgrade.c pacman_LDADD = \ $(LTLIBINTL) \ + libbasic.la \ $(top_builddir)/lib/libalpm/.libs/libalpm.la \ $(LIBARCHIVE_LIBS) -pacman_conf_SOURCES = pacman-conf.c \ - util.h \ - util.c \ - ini.h \ - ini.c \ - util-common.h \ - util-common.c \ - callback.h \ - callback.c \ - conf.h \ - conf.c +pacman_conf_SOURCES = pacman-conf.c -pacman_conf_LDADD = $(top_builddir)/lib/libalpm/.libs/libalpm.la +pacman_conf_LDADD = \ + libbasic.la \ + $(top_builddir)/lib/libalpm/.libs/libalpm.la -- 2.18.0
On 6/7/18 12:42 am, Dave Reisner wrote:
This is shared between pacman and pacman-conf (and might be used by other binaries in the future) -- no need to compile it once for each consumer. ---
I don't understand this patch... (before) Making all in src/pacman CC check.o CC conf.o CC database.o CC deptest.o CC files.o CC ini.o CC package.o CC pacman.o CC query.o CC remove.o CC sighandler.o CC sync.o CC callback.o CC upgrade.o CC util.o CC util-common.o CCLD pacman CC pacman-conf.o CCLD pacman-conf (after) Making all in src/pacman CC conf.lo CC ini.lo CC callback.lo CC util.lo CC util-common.lo CCLD libbasic.la CC check.o CC database.o CC deptest.o CC files.o CC package.o CC pacman.o CC query.o CC remove.o CC sighandler.o CC sync.o CC upgrade.o CCLD pacman CC pacman-conf.o CCLD pacman-conf Nothing is compiled twice there. ini.c and util-common.c are still compiled twice - once in lib/libalpm and once in src/pacman. How is the library helping? Some gain in linking time? A
Use implicit dependency rules to translate asciidoc inputs to HTML and manpage outputs. We should only have to declare explicit dependencies for odd cases, e.g. the PKGBUILD documentation has an additional include file and isn't a 1:1 conversion. --- doc/Makefile.am | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/doc/Makefile.am b/doc/Makefile.am index 8dec4ab1..2ac38cba 100644 --- a/doc/Makefile.am +++ b/doc/Makefile.am @@ -3,7 +3,7 @@ # files listed in EXTRA_DIST no matter what. However, we only add them to # man_MANS if --enable-asciidoc and/or --enable-doxygen are used. -ASCIIDOC_MANS = \ +MANPAGES = \ alpm-hooks.5 \ pacman.8 \ makepkg.8 \ @@ -66,11 +66,11 @@ EXTRA_DIST = \ submitting-patches.asciidoc \ translation-help.asciidoc \ Doxyfile \ - $(ASCIIDOC_MANS) \ + $(MANPAGES) \ $(DOXYGEN_MANS) # Files that should be removed, but which Automake does not know. -MOSTLYCLEANFILES = *.xml $(ASCIIDOC_MANS) $(HTML_DOCS) repo-remove.8 website.tar.gz +MOSTLYCLEANFILES = *.xml $(MANPAGES) $(HTML_DOCS) repo-remove.8 website.tar.gz # Ensure manpages are fresh when building a dist tarball dist-hook: @@ -85,7 +85,7 @@ REAL_PACKAGE_VERSION = $(PACKAGE_VERSION) endif man_MANS = -dist_man_MANS = $(ASCIIDOC_MANS) +dist_man_MANS = $(MANPAGES) if USE_DOXYGEN man_MANS += $(DOXYGEN_MANS) @@ -96,6 +96,7 @@ doxygen.in: $(DOXYGEN) $(srcdir)/Doxyfile endif +man: $(MANPAGES) html: $(HTML_DOCS) website: website.tar.gz @@ -129,11 +130,12 @@ A2X_OPTS = \ -f manpage \ --xsltproc-opts='-param man.endnotes.list.enabled 0 -param man.endnotes.are.numbered 0' -# These rules are due to the includes and files of the asciidoc text -$(ASCIIDOC_MANS): asciidoc.conf footer.asciidoc Makefile.am +# Generate manpages +%: %.asciidoc asciidoc.conf footer.asciidoc Makefile.am $(AM_V_GEN)a2x $(A2X_OPTS) --asciidoc-opts="$(ASCIIDOC_OPTS) --out-file=./$@.xml" $(srcdir)/$@.asciidoc -%.html: %.asciidoc +# Generate HTML pages +%.html: %.asciidoc asciidoc.conf footer.asciidoc Makefile.am $(AM_V_GEN)asciidoc $(ASCIIDOC_OPTS) -o - $*.asciidoc | \ sed -e 's/\r$$//' > $@ @@ -142,27 +144,15 @@ HACKING.html: ../HACKING sed -e 's/\r$$//' > $@ # Customizations for certain HTML docs -$(HTML_MANPAGES): asciidoc.conf footer.asciidoc Makefile.am -$(HTML_OTHER): asciidoc.conf Makefile.am %.html: ASCIIDOC_OPTS += -a linkcss -a toc -a icons -a max-width=960px -a stylesheet=asciidoc-override.css %.8.html: ASCIIDOC_OPTS += -d manpage %.5.html: ASCIIDOC_OPTS += -d manpage %.3.html: ASCIIDOC_OPTS += -d manpage -# Dependency rules -alpm-hooks.5 alpm-hooks.5.html: alpm-hooks.5.asciidoc -pacman.8 pacman.8.html: pacman.8.asciidoc -makepkg.8 makepkg.8.html: makepkg.8.asciidoc -makepkg-template.1 makepkg-template.1.html: makepkg-template.1.asciidoc -repo-add.8 repo-add.8.html: repo-add.8.asciidoc -vercmp.8 vercmp.8.html: vercmp.8.asciidoc -pkgdelta.8 pkgdelta.8.html: pkgdelta.8.asciidoc -pacman-key.8 pacman-key.8.html: pacman-key.8.asciidoc +# Custom dependency rules PKGBUILD.5 PKGBUILD.5.html: PKGBUILD.5.asciidoc PKGBUILD-example.txt -makepkg.conf.5 makepkg.conf.5.html: makepkg.conf.5.asciidoc -pacman.conf.5 pacman.conf.5.html: pacman.conf.5.asciidoc -libalpm.3 libalpm.3.html: libalpm.3.asciidoc -# this one is just a symlink + +# Manpages as symlinks repo-remove.8: repo-add.8 $(RM) repo-remove.8 $(LN_S) repo-add.8 repo-remove.8 -- 2.18.0
This introduces code which has long been maintained at: https://github.com/falconindy/expac
From the README:
expac is a data extraction tool for alpm databases. It features
printf-like flexibility and aims to be used as a simple tool for other
pacman based utilities which don't link against the library. It uses
pacman.conf as a config file for locating and loading your local and
sync databases.
---
I expect that I'll get some pushback on the use of #pragma here. I don't
really know how portable it is to other compilers. Open to suggestions
on how to better avoid this (though I'd prefer not to disable the
warnings entirely for the file).
doc/Makefile.am | 7 +-
doc/expac.1.asciidoc | 179 +++++++++
src/pacman/.gitignore | 2 +
src/pacman/Makefile.am | 8 +-
src/pacman/expac.c | 864 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 1057 insertions(+), 3 deletions(-)
create mode 100644 doc/expac.1.asciidoc
create mode 100644 src/pacman/expac.c
diff --git a/doc/Makefile.am b/doc/Makefile.am
index 2ac38cba..38f7077b 100644
--- a/doc/Makefile.am
+++ b/doc/Makefile.am
@@ -16,7 +16,8 @@ MANPAGES = \
makepkg.conf.5 \
pacman.conf.5 \
libalpm.3 \
- BUILDINFO.5
+ BUILDINFO.5 \
+ expac.1
DOXYGEN_MANS = $(wildcard man3/*.3)
@@ -32,7 +33,8 @@ HTML_MANPAGES = \
PKGBUILD.5.html \
makepkg.conf.5.html \
pacman.conf.5.html \
- libalpm.3.html
+ libalpm.3.html \
+ expac.1.html
HTML_OTHER = \
index.html \
@@ -61,6 +63,7 @@ EXTRA_DIST = \
pacman.conf.5.asciidoc \
BUILDINFO.5.asciidoc \
libalpm.3.asciidoc \
+ expac.1.asciidoc \
footer.asciidoc \
index.asciidoc \
submitting-patches.asciidoc \
diff --git a/doc/expac.1.asciidoc b/doc/expac.1.asciidoc
new file mode 100644
index 00000000..1c7052b5
--- /dev/null
+++ b/doc/expac.1.asciidoc
@@ -0,0 +1,179 @@
+expac(1)
+=========
+
+Name
+----
+expac - alpm data extraction utility
+
+Synopsis
+--------
+'expac' <format>
On 07/05/18 at 10:42am, Dave Reisner wrote:
This introduces code which has long been maintained at:
https://github.com/falconindy/expac
From the README:
expac is a data extraction tool for alpm databases. It features printf-like flexibility and aims to be used as a simple tool for other pacman based utilities which don't link against the library. It uses pacman.conf as a config file for locating and loading your local and sync databases. --- I expect that I'll get some pushback on the use of #pragma here. I don't really know how portable it is to other compilers. Open to suggestions on how to better avoid this (though I'd prefer not to disable the warnings entirely for the file).
I'm not thrilled about adding #pragma to our codebase, but I don't see a better solution. I would think it would be at least as portable as the function attributes we use elsewhere. Just looking at expac.c, I don't see anything major, although I didn't look all that hard because I assume expac is pretty well tested at this point. Since this is a previously external project, I'm fine if you want to incorporate changes as further patches to keep this one as close to original as possible.
doc/Makefile.am | 7 +- doc/expac.1.asciidoc | 179 +++++++++ src/pacman/.gitignore | 2 + src/pacman/Makefile.am | 8 +- src/pacman/expac.c | 864 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 1057 insertions(+), 3 deletions(-) create mode 100644 doc/expac.1.asciidoc create mode 100644 src/pacman/expac.c
...
diff --git a/src/pacman/expac.c b/src/pacman/expac.c new file mode 100644 index 00000000..332414a0 --- /dev/null +++ b/src/pacman/expac.c @@ -0,0 +1,864 @@
...
+ +typedef enum package_corpus_t { + CORPUS_LOCAL, + CORPUS_SYNC, + CORPUS_FILE, +} package_corpus_t;
Is there a reason you don't use the PKG_FROM constants from alpm? ...
+static int print_time(time_t timestamp) { + char buffer[64]; + int out = 0; + + if(!timestamp) { + if(opt_verbose) { + out += printf("None"); + } + return out; + } + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" + /* no overflow here, strftime prints a max of 64 including null */ + strftime(&buffer[0], 64, opt_timefmt, localtime(×tamp)); +#pragma GCC diagnostic pop + + out += printf("%s", buffer);
I'm still trying to figure out how how the hell to correctly use strftime with its multiple undefined behaviors and lack of an error indicator, but we should at least skip the printf if it returns 0. In that case, either there is nothing to print or buffer contains garbage. ...
+static int read_targets_from_file(FILE *in, alpm_list_t **targets) +{ + char line[BUFSIZ]; + int i = 0, end = 0, targets_added = 0; + + while(!end) { + line[i] = fgetc(in); + + if(feof(in)) + end = 1; + + if(isspace(line[i]) || end) {
This should be line-delimited to match pacman rather than space-delimited.
+ line[i] = '\0'; + /* avoid adding zero length arg, if multiple spaces separate args */ + if(i > 0) { + if(!alpm_list_find_str(*targets, line)) { + *targets = alpm_list_add(*targets, strdup(line)); + } + i = 0; + ++targets_added; + } + } else { + ++i; + if(i >= BUFSIZ) { + fprintf(stderr, "error: buffer overflow on stdin\n"); + return -1; + } + } + }
...
On Mon, Jul 09, 2018 at 06:37:35PM -0400, Andrew Gregory wrote:
On 07/05/18 at 10:42am, Dave Reisner wrote:
This introduces code which has long been maintained at:
https://github.com/falconindy/expac
From the README:
expac is a data extraction tool for alpm databases. It features printf-like flexibility and aims to be used as a simple tool for other pacman based utilities which don't link against the library. It uses pacman.conf as a config file for locating and loading your local and sync databases. --- I expect that I'll get some pushback on the use of #pragma here. I don't really know how portable it is to other compilers. Open to suggestions on how to better avoid this (though I'd prefer not to disable the warnings entirely for the file).
I'm not thrilled about adding #pragma to our codebase, but I don't see a better solution. I would think it would be at least as portable as the function attributes we use elsewhere.
Just looking at expac.c, I don't see anything major, although I didn't look all that hard because I assume expac is pretty well tested at this point. Since this is a previously external project, I'm fine if you want to incorporate changes as further patches to keep this one as close to original as possible.
Happy to make most of these changes up front since your suggested changes are fairly minor and very reasonable. Thanks for the review.
doc/Makefile.am | 7 +- doc/expac.1.asciidoc | 179 +++++++++ src/pacman/.gitignore | 2 + src/pacman/Makefile.am | 8 +- src/pacman/expac.c | 864 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 1057 insertions(+), 3 deletions(-) create mode 100644 doc/expac.1.asciidoc create mode 100644 src/pacman/expac.c
...
diff --git a/src/pacman/expac.c b/src/pacman/expac.c new file mode 100644 index 00000000..332414a0 --- /dev/null +++ b/src/pacman/expac.c @@ -0,0 +1,864 @@
...
+ +typedef enum package_corpus_t { + CORPUS_LOCAL, + CORPUS_SYNC, + CORPUS_FILE, +} package_corpus_t;
Is there a reason you don't use the PKG_FROM constants from alpm?
Nope. It's fine to reuse alpm_pkgfrom_t here.
...
+static int print_time(time_t timestamp) { + char buffer[64]; + int out = 0; + + if(!timestamp) { + if(opt_verbose) { + out += printf("None"); + } + return out; + } + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" + /* no overflow here, strftime prints a max of 64 including null */ + strftime(&buffer[0], 64, opt_timefmt, localtime(×tamp)); +#pragma GCC diagnostic pop + + out += printf("%s", buffer);
I'm still trying to figure out how how the hell to correctly use strftime with its multiple undefined behaviors and lack of an error indicator, but we should at least skip the printf if it returns 0. In that case, either there is nothing to print or buffer contains garbage.
...
Sounds good, though silently failing isn't my favorite. At least the glibc implementation of strftime doesn't leave garbage in the buffer on overflow. It'd also be lousy to print something like "error: timefmt overflow" on every single error. I've wanted for a while to do more sanity checking of the format strings up front so that we can fail hard (and once) before gathering packages and printing the results. I guess this is another situation that would fix. For now, let's just go with your suggestion and avoid printing an undefined buffer on failure. In nearly 8 years (I can't believe it's been that long) no one has complained that they've experienced surprising behavior as a result of overflowing the 64 byte time format.
+static int read_targets_from_file(FILE *in, alpm_list_t **targets) +{ + char line[BUFSIZ]; + int i = 0, end = 0, targets_added = 0; + + while(!end) { + line[i] = fgetc(in); + + if(feof(in)) + end = 1; + + if(isspace(line[i]) || end) {
This should be line-delimited to match pacman rather than space-delimited.
Done.
+ line[i] = '\0'; + /* avoid adding zero length arg, if multiple spaces separate args */ + if(i > 0) { + if(!alpm_list_find_str(*targets, line)) { + *targets = alpm_list_add(*targets, strdup(line)); + } + i = 0; + ++targets_added; + } + } else { + ++i; + if(i >= BUFSIZ) { + fprintf(stderr, "error: buffer overflow on stdin\n"); + return -1; + } + } + }
...
This introduces code which has long been maintained at: https://github.com/falconindy/expac
From the README:
expac is a data extraction tool for alpm databases. It features
printf-like flexibility and aims to be used as a simple tool for other
pacman based utilities which don't link against the library. It uses
pacman.conf as a config file for locating and loading your local and
sync databases.
---
v2:
* use alpm_pkgfrom_t instead of homegrown enum
* silently skip output when strftime fails
* split input from stdin on newlines only
doc/Makefile.am | 7 +-
doc/expac.1.asciidoc | 179 +++++++++
src/pacman/.gitignore | 2 +
src/pacman/Makefile.am | 8 +-
src/pacman/expac.c | 858 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 1051 insertions(+), 3 deletions(-)
create mode 100644 doc/expac.1.asciidoc
create mode 100644 src/pacman/expac.c
diff --git a/doc/Makefile.am b/doc/Makefile.am
index 2ac38cba..38f7077b 100644
--- a/doc/Makefile.am
+++ b/doc/Makefile.am
@@ -16,7 +16,8 @@ MANPAGES = \
makepkg.conf.5 \
pacman.conf.5 \
libalpm.3 \
- BUILDINFO.5
+ BUILDINFO.5 \
+ expac.1
DOXYGEN_MANS = $(wildcard man3/*.3)
@@ -32,7 +33,8 @@ HTML_MANPAGES = \
PKGBUILD.5.html \
makepkg.conf.5.html \
pacman.conf.5.html \
- libalpm.3.html
+ libalpm.3.html \
+ expac.1.html
HTML_OTHER = \
index.html \
@@ -61,6 +63,7 @@ EXTRA_DIST = \
pacman.conf.5.asciidoc \
BUILDINFO.5.asciidoc \
libalpm.3.asciidoc \
+ expac.1.asciidoc \
footer.asciidoc \
index.asciidoc \
submitting-patches.asciidoc \
diff --git a/doc/expac.1.asciidoc b/doc/expac.1.asciidoc
new file mode 100644
index 00000000..f5ec3adf
--- /dev/null
+++ b/doc/expac.1.asciidoc
@@ -0,0 +1,179 @@
+expac(1)
+=========
+
+Name
+----
+expac - alpm data extraction utility
+
+Synopsis
+--------
+'expac' <format>
On 11/07/18 03:58, Dave Reisner wrote:
This introduces code which has long been maintained at:
https://github.com/falconindy/expac
From the README:
expac is a data extraction tool for alpm databases. It features printf-like flexibility and aims to be used as a simple tool for other pacman based utilities which don't link against the library. It uses pacman.conf as a config file for locating and loading your local and sync databases.
Where are the plans to use this in the pacman codebase? I assume use in makepkg? I need such a justification given we just booted out every other utility except pacman-conf, which is used in several of our tools (bash/zsh completion, pacman-key, pacman-db-upgrade) and removed many bugs. The only place I know where it will be "useful" is replacing the awk statement for generating .BUILDINFO in makepkg. But that is not fixing a bug. A
participants (4)
-
Allan McRae
-
Andrew Gregory
-
Dave Reisner
-
Dave Reisner