Hi, I'll try to answer your questions :)
On 03/28/2018 10:51 AM, Antoine Beaupré wrote: > On 2018-03-27 21:15:35, Jason Pleau wrote: >> Hi. >> >> I took a few hours last weekend to work on this. > > Awesome, thanks for the work! > >> While I was able to have "working" packages for both xpp and i3ipcpp, >> I could not get polybar to use them (the whole thing is glued together >> nicely it seems and trying to split them caused me headaches). So I >> went ahead and worked on packaging the whole repo (and submodules) >> together. > > Can you expand on the problems you've encountered? Sure. Basically, by itself xpp, it's a header-only library. To build a project using it you have to include xpp's CMakeLists.txt from cmake, which then includes other cmake modules to find the different XCB modules. xpp's CMake file then generates the appropriate XCB_* libraries for your own project (in this case, polybar) to link against. So, if I was to take "xpp" into it's own package, polybar currently would have no way to know which XCB libraries to link against, since it uses xpp's own CMakeFile to find those. That's where I was stuck (I don't really use CMake so this stuff is out of my reach for the time being) > >> Repo: https://salsa.debian.org/jpleau-guest/polybar >> >> Current status: it builds in a chroot and works on my sid install. > > I have tried to build this in stretch and failed: > > $ sbuild -c stretch > dh clean --buildsystem=cmake --builddirectory=build > dh_auto_clean -O--buildsystem=cmake -O--builddirectory=build > dh_autoreconf_clean -O--buildsystem=cmake -O--builddirectory=build > dh_clean -O--buildsystem=cmake -O--builddirectory=build > dpkg-source: error: can't build with source format '3.0 (quilt)': no upstream > tarball found at ../polybar_3.1.0.orig.tar.{bz2,gz,lzma,xz} > E: Failed to package source directory /home/anarcat/dist/polybar > 1$ uscan > uscan warn: No watch file found > 1$ gbp buildpackage -c stretch > dh clean --buildsystem=cmake --builddirectory=build > dh_auto_clean -O--buildsystem=cmake -O--builddirectory=build > dh_autoreconf_clean -O--buildsystem=cmake -O--builddirectory=build > dh_clean -O--buildsystem=cmake -O--builddirectory=build > gbp:error: upstream/3.1.0 is not a valid treeish > > So a few things here: > > * a debian/watch file would be useful, even if just to find out new > versions are coming out... > * the upstream tree should be tagged > > When those are fixed, I get this: > > sbuild-build-depends-polybar-dummy : Depends: debhelper (>= 11) but it is > not going to be installed > > So it might also be useful to make the DH dependency >= 11~ to allow for > easier backporting. I can send a merge request for that on Salsa (or a > patch here) if you want. > 1) watch file: I can add the watch file, if only to check for new versions. 2) debhelper version: sure, I didn't really test building for stretch but I see no harm in changing the version for debhelper if it means polybar can be backported (no need to do a MR I'll take care of it) >> TODO: >> >> - There's a few copyright info missing (ie: lib/concurrentqueue)- > > Seems to be 2-clause BSD. Yep, I saw it but I didn't add the changes yet to debian/copyright (hence the TODO title!) > >> - After installing the package, it won't do anything because the config >> file is not found (it should be in $HOME/.config/polybar). There is one >> shipped in /usr/share/doc/polybar/config.gz, but surely there's a way to >> tell the users that when they install the package? > > /usr/share/doc/polybar/README.Debian is usually where I would expect > that kind of information to be, or in the manpage, or in the error > message directly.. Also, I would expect to find the config.gz file in an > examples/ subdirectory there. > > Maybe a more long-term solution would be to ship the sample config file > in /etc/polybar/config and patch the package to look for there on top of > $XDG_CONFIG_HOME. The proper place to look for those is XDG_CONFIG_DIRS, > which defaults to /etc/xdg, which I've always found weird. See this spec > for details: > > https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html Yes I think upstream would be in favor of having a "system-wide" config file to use as fallback. /etc/polybar or /etc/polybar/config, I'll have to see what other programs do. > >> Note that I made a custom get-orig-source rule. The tarball didn't >> contain xpp and i3ipcpp (github generated tarballs don't include >> submodules). It seems to work fine, feedback welcome on this one.. > > hmm... that does look kind of nasty. :p Why is the version number > hardcoded in $GIT_VER? Why not just use DEB_VERSION_UPSTREAM there? Initially I was using DEB_VERSION_UPSTREAM, but then I thought what if I needed to package a version that has no release (ie: a git revision). Doing it this way lets me use either DEB_VERSION_UPSTREAM, a git tag, or a specific commit. > > It's fine for testing now, but I doubt this tarball would pass NEW: we'd > need to split it into those three packages, probably...? The reason I did it this way was of course because it was easier, and the whole thing seems to "work". I find it nasty as well, but I looked at a few examples before going ahead and do it that way, namely pyopencl: https://sources.debian.org/src/pyopencl/2018.1.1-1/debian/rules/#L60 I found it via codesearch: https://codesearch.debian.net/search?q=git+submodule+path%3Adebian%2Frules I chose the easy route here, because I wanted something that would work. But if we can get this cmake stuff resolved with the extra two packages I have no objection of splitting into three packages. > > Also: when we mess around with the tarballs like this, we usually tag > the upstream version number accordingly, say with "+dfsg1" or > something. In this case, it's not because of the DFSG, but still - we > shouldn't make this package look like upstream, otherwise it brings > confusion to the ecosystem, because the checksums don't match > upstream. I can add a +ds in the version number. So it could be: 3.1.0+ds-1 ? I wasn't certain because the package I was looking at (pyopencl) doesn't do it, but I see no harm in adding a +ds tag. > > At the very least, this stuff should be documented in debian/README.source. Agreed, > > Final nitpicks on the package: > > * the changelog should close this ITP > * please follow DEP3 patch tagging guidelines to explain if patches > were sent back upstream, if so where, and if not why. :) > > http://dep.debian.net/deps/dep3/ The bug number is my mistake I should have added the correct number. It will be fixed. There is one patch where I took a commit from xpp's repository, this one is probably fine, but I will add the DEP3 headers for the json version patch. Ideally this package should be updated in debian as it's a bit out-dated (we have 1.7.4, 1.8.4 is latest upstream) > > Also, I am having trouble making the thing work meaningfully. It seems > it requires quite a bit of configuration... Here's what the default > config gives me by default: > > warn: No monitor specified, using "DP1" > error: Disabling module "bspwm" (reason: Could not find socket: > /tmp/bspwm_0_0-socket) > error: module/xbacklight: Could not get data (err: XCB_NAME (15)) > error: Disabling module "xbacklight" (reason: Not supported for "DP1") > error: Disabling module "wlan" (reason: Invalid network interface "net1") > error: Disabling module "battery" (reason: No suitable way to get current > charge state) > warn: Systray selection already managed (window=0x300000c) > warn: Dropping unmatched character (U+e096) > warn: Dropping unmatched character (U+e099) > warn: Dropping unmatched character (U+e09a) > warn: Dropping unmatched character (U+e09c) > warn: Dropping unmatched character (U+e26f) > warn: Dropping unmatched character (U+e028) > warn: Dropping unmatched character (U+e026) > warn: Dropping unmatched character (U+e19c) > warn: Dropping unmatched character (U+e0ca) > warn: Dropping unmatched character (U+e10c) > > Now a bunch of those are normal: I didn't specify a monitor, I don't > have a working xbacklight, no wlan and no battery. But I have no idea > what's going on with that "unmatched character": this floods my logs and > makes this rather ... difficult to use. It looks like there's something > seriously wrong in the unicode in the default config file: > > $ grep 'format-prefix = ' doc/config.cmake | sed 's/[^"]*"//;s/"//'| head -1 > | hd > 00000000 ee 89 af 20 0a |... .| > 00000005 > $ unicode $(grep 'format-prefix = ' doc/config.cmake | sed > 's/[^"]*"//;s/"//'| head -1 ) > U+E26F - No such unicode character name in database > UTF-8: ee 89 af UTF-16BE: e26f Decimal:  Octal: \0161157 > () > Uppercase: E26F > Category: Co (Other, Private Use) > Unicode block: E000..F8FF; Private Use Area > Bidi: L (Left-to-Right) > > Now, maybe it's me missing something obvious here... Maybe a font? in > that case why would cairo complain that way? > > For what it's worth, i have fonts-font-awesome installed, which should > provide proper fallbacks for those fonts. I get the above even when > adding the font in the font list config. Obviously, this makes the whole > bar look rather bland... :) > > Anyways, I guess that's an upstream problem as well, but it does seem > like the default font provided in the example config file (Siji) is not > in Debian, so that might be a nice addition. :) I will look at the encoding issue. As for the font I don't mind doing the work of packaging, but I'd like to get a working package of polybar first of course. I also have no issue with shipping a custom config with fonts we have in Debian if need be. > > Thanks for the package! Thanks for the comments:) > > A. > -- Jason Pleau
