ftp/wzdftpd: fix build with LLVM 15

[fuz]

Summary

ftp/wzdftpd: fix build with LLVM 15

Seems like the compiler got a whole lot stricter with this release.
The biggest fix was changing the thread ID from unsigned long to
pthread_t, which will probably break compilation on Win32.

While we are at it, replace static uses of REINPLACE_CMD with patch
files as per policy.  This change is responsible for most of the
newly added patches.

Test Plan

Tested on i386 amd64 FreeBSD 12.4 13.1. arm64 pending.
See http://fuz.su/~fuz/freebsd/batch1/

[flo]

Hmm, I'm not sure replacing a couple of REINPLACE_CMD with loads of patch files is a good idea. It would follow the policy, but also add a lot of churn. I'm unsure, @eduardo what do you think?

[fuz]

In D38523#876887, @flo wrote:

Hmm, I'm not sure replacing a couple of REINPLACE_CMD with loads of patch files is a good idea. It would follow the policy, but also add a lot of churn. I'm unsure, @eduardo what do you think?

I've done this change for three reasons: (a) it's in accordance with policy and I was previously dinged for not performing this change (b) some of the REINPLACE_CMD calls no longer apply (c) I had to patch files that were also touched by REINPLACE_CMD, which is always unintuitive. Please let me know if it is incorrect to follow policy in this case.

[eduardo]

Well we use SED cmds to avoid having too much patches, when appliable of course.
In this case we are going backwards.

My opinion is to keep reinplace cmds and improve them if needed.

[fuz]

In D38523#877022, @eduardo wrote:

Well we use SED cmds to avoid having too much patches, when appliable of course.
In this case we are going backwards.

My opinion is to keep reinplace cmds and improve them if needed.

Please see § 4.4.3 Porter's Handbook:

Only use sed(1) to replace variable content. You must use patch files instead of sed(1) to replace static content.

I don't see much wiggle room with “you must use.”

[eduardo]

In D38523#877025, @fuz_fuz.su wrote:

In D38523#877022, @eduardo wrote:

Well we use SED cmds to avoid having too much patches, when appliable of course.
In this case we are going backwards.

My opinion is to keep reinplace cmds and improve them if needed.

Please see § 4.4.3 Porter's Handbook:

Only use sed(1) to replace variable content. You must use patch files instead of sed(1) to replace static content.

I don't see much wiggle room with “you must use.”

Lets see an example:

-libdir = ${pkgdatadir}/{backends,modules}
+libdir = ${prefix}/lib/wzdftpd/{backends,modules}

Creates about 10 file patches where one simple sed cmd could do that job.
Thats where that reinplace cmds enter dispite of variable/static content.

[fuz]

Creates about 10 file patches where one simple sed cmd could do that job.
Thats where that reinplace cmds enter dispite of variable/static content.

And it's in violation of policy and harder to maintain for me, the maintainer, to maintain. I believe this is a more important argument than saving a little disk and inode space.

[eduardo]

In D38523#877029, @fuz_fuz.su wrote:

Creates about 10 file patches where one simple sed cmd could do that job.
Thats where that reinplace cmds enter dispite of variable/static content.

And it's in violation of policy and harder to maintain for me, the maintainer, to maintain. I believe this is a more important argument than saving a little disk and inode space.

I recomend using reinplace cmds where it applies (like example above) and patches where stricly needed.

[fuz]

I understand. Does that mean you will not approve of this patch for this reason?

[eduardo]

In D38523#877084, @fuz_fuz.su wrote:

I understand. Does that mean you will not approve of this patch for this reason?

I believe it can be improved with both sed and patches to reduce ~25 adicional patches.
I understand policy, but I also understand that I should not follow it blindly.

[fuz]

The thing is, over the past 1½ years maintaining ports, I have constantly received contradicting guidance. One committer wants me to do it this way, the next one tells me this is wrong and wants me to do it another way. One tells me to follow policy, the next tells me to ignore policy. One tells me that something has to be done a certain way, despite a large amount of ports and commits not actually doing it that way. Then another tells me that it either doesn't matter or that I should do it another way. This is honestly extremely frustrating and I really don't like revising my patches again and again because of things that boil down to personal preferences of other committers.

The purpose of mentorship is to ensure that the contributions are up to the standards and conventions of the FreeBSD project and that there's nothing wrong with them. I believe this is a very important objective and am very grateful for the support of you and flo@ in pursuing this endeavour. However, things that boil down to personal preference (such as whether to use REINPLACE_CMD or patch files for this case, or whether to capitalise after the colon of the commit message) are not actually wrong or in contradiction to the standards of the FreeBSD project.

So I must kindly ask you to consider if something you would do differently in a commit is because of your personal preference, or because it is actually wrong or detrimental in some way. I will invariably violate your personal preferences in various ways because many other committers have previously tought me to prepare commits in accordance with their own personal preferences. Does that mean these choices are wrong and must be corrected? If I have researched that it is generally accepted practice by many committers to do things a certain way and then get dinged by you for doing it that way, that is very discouraging.

However, as for things that are actually wrong or outdated, please let me know so I can improve these. I am also thankful for you build-testing all my patches to ensure nothing slips by.

[eduardo]

I understand what you say and no need to be frustrated about it.
This is a good to practice example about SED versus patches.

I cannot accept the same thing repeated over 10 file patches, I did not look at the rest of them: ~15 patches, without a better solution.

[fuz]

I'll have to think of something. I am not going to put the REINPLACE_CMD back in. Worst case I'll have to step back from maintainership.

[mat]

In D38523#877102, @eduardo wrote:

I understand what you say and no need to be frustrated about it.
This is a good to practice example about SED versus patches.

I cannot accept the same thing repeated over 10 file patches, I did not look at the rest of them: ~15 patches, without a better solution.

Well, using REINPLACE_CMD for this is deprecated and should not be used. For the exact reasons cited earlier. When a file gets changed, and a sed becomes obsolete or useless, there is nothing to notify the maintainer that it does nothing any more.
The only reason to use REINPLACE_CMD is because you are replacing with non static content, like some variable.

The patch provided here is just fine, the fact that you don't like it does not really matter, it is correct.

What could be done is group all the files related to the reinplace_cmd removal into one patch, like, files/patch-libdir, and patch-gcrypt or such, it would lower the number of files.

[fuz]

Added a hack to avoid lots of Makefile.am patches in accordance with
policy and mentor guidance. Other patches left untouched as they do
not follow any obvious pattern.

Proposed commit message:

ftp/wzdftpd: fix build with LLVM 15

Seems like the compiler got a whole lot stricter with this release.
The biggest fix was changing the thread ID from unsigned long to
pthread_t, which will probably break compilation on Win32.

While we are at it, replace static uses of REINPLACE_CMD with patch
files as per policy.  This change is responsible for most of the
newly added patches.

The wzdftpd build scripts want to install plugins into ${PREFIX}/share
when they belong into ${PREFIX}/lib.  Instead of patching this in each
Makefile.am, set --datadir=${PREFIX}/lib and work around the one file
for which this is wrong.

Approved by:    ... (mentor)

[eduardo]

In D38523#877110, @mat wrote:

In D38523#877102, @eduardo wrote:

I understand what you say and no need to be frustrated about it.
This is a good to practice example about SED versus patches.

I cannot accept the same thing repeated over 10 file patches, I did not look at the rest of them: ~15 patches, without a better solution.

Well, using REINPLACE_CMD for this is deprecated and should not be used. For the exact reasons cited earlier. When a file gets changed, and a sed becomes obsolete or useless, there is nothing to notify the maintainer that it does nothing any more.
The only reason to use REINPLACE_CMD is because you are replacing with non static content, like some variable.

The patch provided here is just fine, the fact that you don't like it does not really matter, it is correct.

What could be done is group all the files related to the reinplace_cmd removal into one patch, like, files/patch-libdir, and patch-gcrypt or such, it would lower the number of files.

Thanks @mat for clarification. That would be too a good solution.

[eduardo]

Nice workaround on patches. Thanks.

[mat]

ftp/wzdftpd/files/patch-libwzd-core_wzd__libmain.c

8

I don't know what this does, but it feels like some initialization, should not it be kept, simply casting to the correct type?

[fuz]

ftp/wzdftpd/files/patch-libwzd-core_wzd__libmain.c

8

Wzdftpd assumes that pthread_t is an integer (i.e. a process ID) which is the case on Linux and some other systems. It therefore mixes and matches pthread_t and unsigned long, something the compiler objects to. Many of these patches try to fix that by consistently treating pthread_t as something that could either be a pointer or an integer.

The whole structure is cleared a few lines above, so without writing -1 to it, the field just stays at zero, which is okay both as an initial value for when pthread_t is a pointer and if it is an integer.