Hello Stuart, thanks for responding so quickly and extensive.

Stuart Henderson wrote [2012-05-02 01:12+0200]:
> On 2012/05/02 00:03, Steffen Daode Nurpmeso wrote:
> > appended is a diff which creates a new port x11/ahwm.
> 
> prefer tar.gz for new ports

That's not documented in faq/ports yet.
I should have looked into marc.info, though.
Trying this time via attachment.

> > Have not figured out SEPARATE_BUILD yet.
> 
> set it to Yes if it works. not worth spending much time on trying to
> make it work unless SEPARATE_BUILD gives a clear advantage (mostly when
> a port is fairly large).

Ok, i have made no further efforts in this regard.

> > I had to implement the do-install: rule for sane sanity.
[.]
> > The patches included are actually his own from the website
> > (ahwm-0.90.switch.patch).
> 
> ahwm.c:174: warning: implicit declaration of function 'strcmp'
> ahwm.c:712: warning: implicit declaration of function 'strlen'

Ouch.  I have even missed to really notice

  ahwm.c:712: warning: incompatible implicit declaration of
  built-in function 'strlen'

So thanks for catching this.

> workspace.c:54: warning: cast from pointer to integer of different size
> workspace.c:106: warning: cast to pointer from integer of different size

These are harmless unless you create that many different
workspaces, but i've added explicit intermediate casts via
ptrdiff_t.

> implicit decl of string functions are rather likely to cause problems on
> LP64 arch. casts from pointer to/from int are sometimes OK but they can
> often cause trouble too. if you haven't tried it on 64-bit it needs
> testing (it's always a good idea to say what arch you've tested on).

i386 (blindly compiled from source).
amd64 not that long (dito).
The thing is - it simply *never* crashed and i have *never* used
that command line argument, so stack pushing never happened.

> > +COMMENT =          plain X11 window manager
> > +CATEGORIES =               x11     
>                            ^^^^^
> trailing whitespace

Fixed.
In fact the Makefile was copied over from icewm, which thus
includes the same slip.

> > +V =                        0.90
> > +DISTNAME =         ahwm-$V
> 
> just use DISTNAME=ahwm-0.90, setting this in a separate var is only useful
> if you reference it later

Ok.

> > +#PKGNAME =         ${DISTNAME}
> > +#WRKDIST =         ${WRKDIR}/${DISTNAME}
> 
> zap

Ok.

> > +MASTER_SITES =             
> > ftp://ftp.freebsd.org/pub/FreeBSD/ports/distfiles/ \
> > +                   ${HOMEPAGE}
> 
> not much point listing the FreeBSD mirror there unless the
> proper master site is very unreliable (and if that's the case
> I'd rather mirror it myself).

I don't know wether it's reliable or not.
But it's a private homepage now, no longer backed by the site of
an university?
I was kinda happy once i discovered the mirror yesterday.
I don't know how the storage problem is solved.
I've removed FreeBSD there.

> > +# Since we override install: it's possible to use gnu-style
> > +CONFIGURE_STYLE =  gnu
> 
> generally it's better to use the upstream install target,
> "CONFIGURE_STYLE=gnu dest" sometimes helps.

Didn't.

> with hand-rolled do-install new files are often missed in version
> updates, but I guess this is not a big problem with this particular port.

Nah.
But you never know, the author is (or at least his forefathers
were) compatriot(s) of Dracula.

That's kind of frightening.

> > +# These are redundant, but be explicit for now
> > +CONFIGURE_ARGS =   --with-x --enable-shape
> 
> leave those out, it's just the --disable / --without lines that you
> really need to list explicitly.

Did so.  (In fact i guess it was wrong since --enable-shape should
then include WANTLIB=Xext.)

> > +do-install:
> > +   ${INSTALL_PROGRAM_DIR} ${PREFIX}/bin
> > +   ${INSTALL_MAN_DIR} ${PREFIX}/man/man5
> 
> no point creating these, they're already created from the mtree

Hmmm, so i did.

> > +   ${INSTALL_PROGRAM} ${WRKSRC}/ahwm ${PREFIX}/bin/
> > +   ${INSTALL_MAN} ${WRKSRC}/ahwmrc.5 ${PREFIX}/man/man5/
> 
> I'd rather have 'man ahwm' working, so I would either keep the link or
> if you're only going to install it under one name, make that ahwm.

Readded symbolic link ahwm.5.

> > +$OpenBSD$
> > +--- move-resize.c.orig     Mon Apr 30 20:03:16 2012
> > ++++ move-resize.c  Mon Apr 30 20:03:43 2012
> 
> it's often good to add a comment after $OpenBSD$ with the source of a
> patch if it came from upstream etc.

And i also did that for the entire patch branch.

Thanks again, Stuart.

--steffen
Forza Figa!

Attachment: ahwm.tar.gz
Description: application/tar-gz

Reply via email to