PKGBUILD review request: PacVim from github repo [update: AUR link]

zero2cx · 2016-12-28 05:03:49

EDIT: Thanks for the review. It was instructive for me. The new AUR package can be found here.

This is my first effort at building a package. When it is ready, I'll submit it for inclusion to the AUR. I have the application PacVim successfully building and installing to my system. ~~However, I am unsure whether this unreleased git project should be versioned in the AUR as 0.r127.beaa700 or maybe simply just r127.beaa700 .~~

Below is the PKGBUILD I've got so far. Please let me know where I can apply any fixes that you might suggest. Thank you.

Note that I have updated the below PKGBUILD with each edit detailed at the bottom of this post.

# Maintainer: zero2cx <zero2cx@gmail.com>

pkgname=pacvim-git
pkgver=v1.1.1.r5.gbeaa700
pkgrel=1
pkgdesc="A game that teaches you vim commands. Inspired by Namco's classic PAC-MAN."
arch=('i686' 'x86_64')
url='https://github.com/jmoon018/PacVim'
license=('LGPL3')
depends=('ncurses')
makedepends=('git')
provides=("${pkgname%-git}")
source=("${pkgname%-git}::git+${url}.git")
md5sums=('SKIP')

pkgver() {
        cd "${srcdir}/${pkgname%-git}"
        git describe --long --tags | sed 's/\([^-]*-g\)/r\1/;s/-/./g'
}

build() {
        cd "${srcdir}/${pkgname%-git}"
        make PREFIX=/usr
}

package() {
        cd "${srcdir}/${pkgname%-git}"
        make PREFIX="${pkgdir}/usr" install
}

EDIT 1: Updated quote-nesting typo in pkgdesc .
EDIT 2: Updated pkgver() so that it will echo the release tagging which I had overlooked.
EDIT 3: Changed the post title and part of my query above to reflect that I had missed the fact that upstream had released this project with tags v1.1.0 and v1.1.1 .
EDIT 4: Updated pkgver() again, removing the unnecessary printf command.

Last edited by zero2cx (2016-12-31 16:08:42)

Scimmia · 2016-12-28 05:22:16

The github page is showing 2 releases. 'git describe' should work, which is good, as the current pkgver function doesn't actually work.

Having a single quote (apostrophe) in the pkgdesc is a problem. EDIT: This was already fixed in the OP's edit.

The upstream makefile sucks. It's overriding CFLAGS, misusing LFLAGS, and plenty of other problems. They'll need to be worked around.

Last edited by Scimmia (2016-12-28 05:40:44)

eschwartz · 2016-12-28 05:42:56

https://wiki.archlinux.org/index.php/VC … elines#Git -- see un-annotated tags, though the # of revisions example also clearly demonstrates the accepted standard for those.

@Scimmia, why is a single quote a problem for a double-quoted string?

That Makefile is indeed rather bad. Consider filing a pull request to fix it.

Scimmia · 2016-12-28 05:46:32

Eschwartz wrote:

Scimmia, why is a single quote a problem for a double-quoted string?

Because it wasn't a double-quoted string. He edited, and I edited.

eschwartz · 2016-12-28 05:49:48

And I didn't notice your edit, sorry.

zero2cx · 2016-12-28 06:26:29

Thanks for the guidance, so far! Very nice. I am improving the OP above, incorporating all the fixes as appropriate.

As I am unsure of Makefile construction and best practices so I can't yet be sure where improvement is needed. I'll begin by studying a bit on the subject. When I am finished in that regard, then I can try to get with the upstream developer to offer a coherent pull request.

I'll be exploring Archwiki resource pages in reference to Makefile, etc. If anyone wishes to point me to resources elsewhere so I can best get myself up to speed on CFLAGS syntax and good habits, etc., I would appreciate that. Thanks again for all the guidance.

Last edited by zero2cx (2016-12-28 06:27:52)

zero2cx · 2016-12-28 22:13:19

As I see it, here are Scimmia's and Eschwartz knocks of the upstream Makefile so far, paraphrased and itemized:

The package maintainer (zero2cx) should consider filing a pull request to work around the following rather bad issues within the upstream Makefile:
it overrides CFLAGS improperly
it misuses LFLAGS
it contains plenty of other problems

Improving that which I am able while referencing the list above, I now have the Makefile you see below with the following listed changes having been made so far (original Makefile here):

replaced the assignment operators for a few of the lines (lines 1, 2, and 10)
use variable LIBS in place of LFLAGS
include the conditional linking of the library file for "pthread" (the compiler would sometimes complain and then fail to link successfully)

SRCS   := $(wildcard src/*.cpp)
OBJS   := $(SRCS:.cpp=.o)

PREFIX = /usr/local
BINDIR = $(PREFIX)/bin
SHARE  = $(PREFIX)/share
MAPDIR = $(SHARE)/pacvim-maps

CXX    = g++
CFLAGS += -std=c++11 -DMAPS_LOCATION='"$(MAPDIR)"'
LIBS   = -lncurses

TARGET = pacvim
MAPS   = maps
DEPS   = .deps

ifneq ($(shell uname -s 2>/dev/null || echo nop),Darwin)
# OS has POSIX threads in libc
CFLAGS += -pthread
LIBS   += -lpthread
endif

all: $(TARGET)

$(TARGET): $(OBJS)
	$(CXX) $(CFLAGS) $^ $(LIBS) -o $@

%.o: %.cpp
	$(CXX) $(CFLAGS) -c -o $@ $<

$(DEPS):
	$(CXX) -M $(SRCS) >| $@

clean:
	$(RM) $(OBJS) $(DEPS)

install: $(TARGET)
	install -d $(BINDIR) $(SHARE)
	install -m 755 $(TARGET) $(BINDIR)/
	cp -r $(MAPS) $(MAPDIR)

uninstall:
	$(RM) $(BINDIR)/$(TARGET)
	$(RM) -r $(MAPDIR)

-include $(DEPS)

I'd be better able to submit a pull request upstream if I could understand what was meant when criticism of the original Makefile does include the vague "plenty of other problems". I do recognize that the "ifneq" conditional test looks to my eye to be broken. Other than that, at this stage I can't be sure what other improvements might be appropriate for me to make next.

~~In any case, can someone here please help me out by letting me know if I am on the right track or not with the changes shown in this post?~~
Thank you.

Last edited by zero2cx (2016-12-30 16:30:54)

Trilby · 2016-12-28 22:20:56

Is this your project, or are you just packaging it. If it is yours, we can definitely help with the Makefile. Also, if it is yours, fix your licensing. The LICENSE file says GPL3, but you do not properly include the GPL3 license which is required for proper licensing.

Also note that the "All rights reserved" statement directly contradicts GPL licensing. While this statement is no longer necessary even on restricted material (it is implied by copyright) the statement only ever served to emphasis that any copying or redistribution was specifically disallowed.

Here's a working Makefile:

TARGET     =  pacvim
PREFIX    ?=  /usr/local
BINDIR     =  $(PREFIX)/bin
MAPDIR     =  $(PREFIX)/share/pacvim-maps
OBJS      :=  $(patsubst %.cpp,%.o,$(wildcard src/*.cpp))
MAPS      :=  $(wildcard maps/*)
CXX       ?=  g++
CXXFLAGS  +=  -std=c++11 -DMAPS_LOCATION='"$(MAPDIR)"'
LDLIBS    +=  -lncurses -lpthread

ifneq ($(shell uname -s 2>/dev/null || echo nop),Darwin)
# OS has POSIX threads in libc
CXXFLAGS += -pthread
endif

$(TARGET): $(OBJS)
	$(CXX) $(LDFLAGS) $(LDLIBS) $^ -o $@

install: $(TARGET)
	install -Dm755 $(TARGET) $(DESTDIR)$(BINDIR)/$(TARGET)
	install -d $(DESTDIR)$(MAPDIR)
	install -t $(DESTDIR)$(MAPDIR) $(MAPS)

uninstall:
	$(RM) $(DESTDIR)$(BINDIR)/$(TARGET)
	$(RM) -r $(DESTDIR)$(MAPDIR)

.PHONY: install uninstall

This appends to rather than overriding user-specified flags. It also uses an implicit rule for object files. An explicit rule is needed for the final binary as there is no source/object file called pacvim. ~~I've changed the prefix to just be usr, but if you want usr/local as a default too that's fine~~ - just set it conditionally as I have done (with ?=) so that if the user specifies a prefix, their preference will be honored. I've also added DESTDIR to the install directive which is needed for packaging.

EDIT: I'm a bit to arch-centric. Scimmia's comment on prefix below is correct. /usr/local is a better default - but the more important point is that it is set conditionally to that default only if the user has not specified a PREFIX.

EDIT 2: fixed LDFLAGS -> LDLIBS

zero2cx · 2016-12-28 23:09:31

Trilby wrote:

Is this your project, or are you just packaging it. If it is yours, we can definitely help with the Makefile.

I am just packaging this. Note that I posted the OP hoping to improve my PKGBUILD and that file now looks and works great, in my opinion.

Scimmia's and Eschwartz's posts above, taken together, did convince me to try to improve upstream's broken Makefile and then submit a pull request upstream to fix the issues that were (never precisely) identified for me in this thread. Really, I am just guessing as to what needs improvement at this point. Maybe nothing? EDIT: I somehow missed that you provided me a working Makefile. I will be submitting my pull request upstream very soon, thanks to you.

Trilby, are you of the opinion that I should just finish packaging this as it is? If I go that route, I could simply address any potential compile problems that Arch users might post about in the pacvim-git AUR page comments as they occur.

Trilby wrote:

Also, if it is yours, fix your licensing. The LICENSE file says GPL3, but you do not properly include the GPL3 license which is required for proper licensing.

As it is now, the LICENSE is not in compliance and I plan to submit a pull request to address that ASAP.

Last edited by zero2cx (2016-12-28 23:12:05)

Scimmia · 2016-12-28 23:36:50

Trilby's Makefile looks much better; the only thing I would change before submitting upstream is to default PREFIX to /usr/local instead of /usr. It's much more typical to default there, and for people to install to /usr/local when just running make/make install manually.

Trilby · 2016-12-29 00:44:00

zero2cx, if you can get upstream to incorporate some of these changes, it will make keeping the PKGBUILD up-to-date much easier. As is, I don't know if the install directive would work at all. Using a different PREFIX for the build and the install is a creative workaround - but I'd not be surprised if it came back to haunt you eventually.

eschwartz · 2016-12-29 00:55:40

+1 to Trilby's Makefile.

I am not great expert myself but I don know a little something.. enough to know when it is wrong, certainly.

You can see Make's implicit rules (which serve as great examples of how to do it right in a standards-conformant manner) using `make -C /dir/without/a/makefile -p|less`
You will see that linking always uses LDFLAGS (and possibly LDLIBS), no idea where LFLAGS came from at all but it certainly won't match the default LDFLAGS in makepkg.conf !
Aside for matching the variables used by most people (and implicit rules) and allowing people to supply their own values, you mostly just need to read enough well-written Makefiles to learn how it works. e.g. DESTDIR for prefixing install paths.

@Trilby, why *do* you use LDFLAGS instead of LDLIBs?

Trilby · 2016-12-29 01:10:56

Eschwartz wrote:

@Trilby, why *do* you use LDFLAGS instead of LDLIBs?

Because I like being wrong from time to time

I've edited my suggested makefile to use usr/local as the default PREFIX and to properly use LDLIBS. LDFLAGS was the result of two influences: 1) it's closer to LFLAGS so I was primed to think LDFLAGS rather than LDLIBS, and 2) it is a mistake I've made in some of my own makefiles as I learned it wrong initially. I think I've fixed it in all of my current makefiles, but old habits die hard.

On another note LFLAGS is an implicit variable name. Just not for a purpose anything like these: it's for Lex flags.

eschwartz · 2016-12-29 01:43:47

As opposed to me, who obsessively copy-pastes from everywhere I can as a replacement for real knowledge.

Thanks for clarifying.

zero2cx · 2016-12-29 05:39:15

Well, thanks to everyone here who's helped me out on the path to properly packaging pacvim-git for submission to the AUR. Especially helpful were those who posted to make me aware that the upstream Makefile sucks. I enjoyed all the input that I found here yet still, Trilby did the heavy lifting by authoring a new Makefile and I really appreciate that!

Also of note, I now have an open pull request upstream that seeks to straighten out the strange state of the release license. Upstream needs to either bring the project terms into GPL3 compliance or else to draw it back it into an "all rights reserved" status, whichever the case shall be. If my license mod is resolved upstream without any problem, I plan on then opening a second pull request to amend upstream's broken Makefile.

Finally, I have a question that shall apply only if pacvim-git ever makes it into the AUR repo. Q: Is Trilby a package "contributor" that is due a mention/comment-line in the PKGBUILD? What is best etiquette?

eschwartz · 2016-12-29 06:08:56

The PKGBUILD is yours, we just helped you figure out how you wanted to write it.

It wouldn't hurt to give Trilby accreditation for the Makefile, in the pull request that is.
You could use his info (GitHub projects are in his signature) for GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL or something. Or maybe that is what the kernel's "Suggested-by: " footers are for.

Trilby · 2016-12-29 12:07:59

I appreciate the thought. But all of the above is for you or the upstream dev to do with as you wish. While I wouldn't necessarily mind following up if there were concerns about how that Makefile works, I'd actually prefer my info wasn't particularly associated with it (if my name is anywhere on/in/associate with the makefile, someone might try to contact me when the software fails).

zero2cx · 2016-12-30 16:21:02

For those users who've expressed a preference for an improved Makefile upstream, please note that I have opened a pull request that addresses the issue. And finally, having readied a working PKGBUILD for pacvim-git up there in the OP, this package now appears to me to be ready to submit for inclusion in the AUR. I will edit the OP with a link to the package web page once pacvim-git is added to that repo. Thanks to all.

Arch Linux

#1 2016-12-28 05:03:49

PKGBUILD review request: PacVim from github repo [update: AUR link]

#2 2016-12-28 05:22:16

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#3 2016-12-28 05:42:56

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#4 2016-12-28 05:46:32

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#5 2016-12-28 05:49:48

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#6 2016-12-28 06:26:29

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#7 2016-12-28 22:13:19

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#8 2016-12-28 22:20:56

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#9 2016-12-28 23:09:31

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#10 2016-12-28 23:36:50

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#11 2016-12-29 00:44:00

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#12 2016-12-29 00:55:40

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#13 2016-12-29 01:10:56

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#14 2016-12-29 01:43:47

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#15 2016-12-29 05:39:15

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#16 2016-12-29 06:08:56

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#17 2016-12-29 12:07:59

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

#18 2016-12-30 16:21:02

Re: PKGBUILD review request: PacVim from github repo [update: AUR link]

Board footer