On 9/6/23 18:14, Omar Polo wrote:
Hello,
On 2023/08/19 10:04:54 +0200, Igor Zornik <[email protected]> wrote:
Greetings ports@,
Long time listener, but first time caller so I hope I didn't get to many
things wrong.
Sorry for the delay. I wanted to review this the day it was sent
but only managed to look at it today.
Attached I have a port of alps, an e-mail web client. It's not as
feature-full as some other web clients, but I like this one because it
has not run-time dependencies which makes it easier to deploy and maintain.
Notes:
- Port initially created with `portgen`. When modified I relied heavily
on gitea port.
- Upstream isn't very responsive to my change proposals so I provided
some fixes and features locally.
- There isn't any special functionality associated with the service
account so I just made it to run as `www`. Should I still run it under a
dedicated service account?
- I had to do some `sed` trick in the `rc` script to get the `rc`
command to pick up the daemon properly. Could anyone confirm if i have
taken the right path here?
- Tested on amd64 using -current and 7.2.
Any comments or suggestions?
a few nits, but otherwise it's a solid submission.
- I'd add some commentary to the patches. Just something brief like
"adding FastCGI support", so that it's clear what it does when
skimming the files.
- I'd drop the patch for go.mod; I'm not sure go.port.mk even looks
at the fetched go.mod file, let alone parsing it once patched.
- there's a small error in the fastcgi patch:
+ addr = strings.TrimPrefix(addr, "fcgi+unix:/")
should be
+ addr = strings.TrimPrefix(addr, "fcgi+unix://")
i.e. remove two slashes. In practice it's not an issue.
- While I really like the fastcgi support (I'm a bit lazy and never
bothered to learn how to use relayd), I don't think we've usually
patched the ports to add features like these. Have you sent the
patch upstream? I can't find it searching for 'fastcgi' on
sourcehut.
To be fair I don't think it's an issue, but still felt like
pointing this out.
- It doesn't work without altering the flags to specify the upstream
server. This has confused me initially, so add that info to the
README.
I've also tweaked the fastcgi example to use `rcctl set alps flags'
and add a dummy example.com at the end to minimize the chance of
getting things wrong.
- (opinable) I've tweaked the http example to listen on port 80.
Otherwise it's not copy-paste-able without further fiddling.
- I'm not sure about ${SYSCONFDIR}/httpd.conf. Even if portcheck
complains, the correct path is /etc/httpd.conf: it is hardcoded in
the httpd(8) binary.
- make update-plist changed @cwd ${LOCALSTATEDIR}/www into /var/www
and I'm not sure how much we care about using the variables instead
of the hardcoded paths. Left as /var for the moment.
- no opinion on the sed machinery to escape '+'s in the pexp. Seems
to work :-)
- changed the version to 0.0.0.<date> so if upstream starts to do
proper releases we don't have to use EPOCH.
I'm reattaching a tarball with these points addressed. It's ok op@ to
import if someone wants to, otherwise i'll be happy to import it
myself (needs an ok to do so).
I don't use web ui for mails if I can, but this is one of the nicer
I've seen.
