Bug 270440 - net/pkt-gen: Update to g2023.03.22
Summary: net/pkt-gen: Update to g2023.03.22
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Olivier Cochard
URL: https://github.com/luigirizzo/netmap/...
Keywords: patch
Depends on:
Blocks:
 
Reported: 2023-03-25 03:35 UTC by Jose Luis Duran
Modified: 2023-04-22 17:11 UTC (History)
1 user (show)

See Also:
bugzilla: maintainer-feedback? (olivier)


Attachments
net/pkt-gen: Update to g2023.03.22 (3.75 KB, patch)
2023-03-25 03:35 UTC, Jose Luis Duran
no flags Details | Diff
Proposed upstream patch to fix compilation on some Tier-2 platforms (1.19 KB, patch)
2023-04-19 17:57 UTC, Jose Luis Duran
no flags Details | Diff
Proposed upstream patch to fix compilation on 32-bit architectures (intmax_t) (1.19 KB, patch)
2023-04-19 20:31 UTC, Jose Luis Duran
no flags Details | Diff
Proposed upstream patch to fix compilation on 32-bit architectures (intmax_t) (1.19 KB, patch)
2023-04-20 01:31 UTC, Jose Luis Duran
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jose Luis Duran 2023-03-25 03:35:45 UTC
Created attachment 241098 [details]
net/pkt-gen: Update to g2023.03.22

Update to the latest commit. It has over a year of improvements.

Changelog:
https://github.com/luigirizzo/netmap/compare/278045b...0a53f85

None of the patches are needed to build it successfully.

I have not tested that it builds on 12.x, looking at bug #232919.
Comment 1 commit-hook freebsd_committer freebsd_triage 2023-03-27 12:12:43 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=9305cd3f9321bfba95479b2b2a15c7639a790383

commit 9305cd3f9321bfba95479b2b2a15c7639a790383
Author:     Olivier Cochard <olivier@FreeBSD.org>
AuthorDate: 2023-03-27 12:10:13 +0000
Commit:     Olivier Cochard <olivier@FreeBSD.org>
CommitDate: 2023-03-27 12:12:12 +0000

    net/pkt-gen: Update to g2023.03.22

    PR:             270440
    Reported by:    Jose Luis Duran <jlduran@gmail.com>

 net/pkt-gen/Makefile                                     |  4 ++--
 net/pkt-gen/distinfo                                     |  6 +++---
 net/pkt-gen/files/patch-apps_nmreplay_GNUmakefile (gone) | 12 ------------
 net/pkt-gen/files/patch-apps_nmreplay_nmreplay.c (gone)  | 11 -----------
 net/pkt-gen/files/patch-apps_pkt-gen_GNUmakefile (gone)  | 12 ------------
 net/pkt-gen/files/patch-apps_pkt-gen_pkt-gen.c (gone)    | 13 -------------
 net/pkt-gen/files/patch-libnetmap_GNUmakefile (gone)     | 11 -----------
 7 files changed, 5 insertions(+), 64 deletions(-)
Comment 2 Olivier Cochard freebsd_committer freebsd_triage 2023-03-27 12:13:14 UTC
Thanks for your patch!
Comment 3 Robert Clausecker freebsd_committer freebsd_triage 2023-04-19 12:05:30 UTC
These patches (especially the one patching out -Werror) were needed to build the port on i386 and armv7.  Now I have to put them back in, for what looks like the third time.  Please don't waste developer time like that.
Comment 4 Jose Luis Duran 2023-04-19 15:18:26 UTC
(In reply to Robert Clausecker from comment #3)
Ouch! I'm sorry, I can relate to your frustration.

When/if you add the patches again, could you please add a comment at the top of the patch explaining that it is required for those platforms?
Comment 5 Robert Clausecker freebsd_committer freebsd_triage 2023-04-19 15:32:23 UTC
(In reply to Jose Luis Duran from comment #4)

I have documented all of this in the commit messages when I added the patches, as is common in the FreeBSD ports collection. You can find them e.g. on Freshports:

    https://www.freshports.org/net/pkt-gen

I will not add additional explanation to the port.  Read the commit logs before changing ports you are not familiar with.  And do not remove patches without understanding why they were added and checking if the reason still applies.  Just because the port builds without certain patches (especially if you only tested one architecture) doesn't mean they are obsolete.  Often patches are only needed for some architectures or fix something that is not a build problem but rather only manifests at runtime.

I appreciate your help with the ports collection, but if you just randomly break ports because you didn't bother to check why things are the way they are, you'll only create more work.

A note on -Werror: it's a horrible option that breaks ports whenever the compiler is updated and feels like issuing a new warning.  Just because the port builds without warnings on one version/architecture of FreeBSD doesn't mean that it always will.  So -Werror is a fun way to create endless work for package maintainers until it is exorcised.  Always patch out -Werror when you see it.  And do not remove patches that patch out -Werror, even if there are no warnings at the moment.
Comment 6 Jose Luis Duran 2023-04-19 16:07:23 UTC
(In reply to Robert Clausecker from comment #5)
> A note on -Werror: it's a horrible option that breaks ports whenever the compiler is updated and feels like issuing a new warning.

I see a somewhat related report upstream:
https://github.com/luigirizzo/netmap/issues/540

Maybe that's the best place to fix it?
Comment 7 Robert Clausecker freebsd_committer freebsd_triage 2023-04-19 16:16:44 UTC
(In reply to Jose Luis Duran from comment #6)

Not sure how.  It's not about getting rid of -Werror.
Comment 8 Jose Luis Duran 2023-04-19 17:32:40 UTC
(In reply to Robert Clausecker from comment #7)
I mean, something like this:

https://github.com/luigirizzo/netmap/compare/master...jlduran:netmap:fix-tier-2-compilation

The overall goal would be to facilitate porting, as well as the syncing of pkt-gen in base.  I assume you are able to build pkt-gen from base (src/tools/tools/netmap) without issues.
Comment 9 Robert Clausecker freebsd_committer freebsd_triage 2023-04-19 17:34:47 UTC
(In reply to Jose Luis Duran from comment #8)

That patch is wrong as tv_nsec is always of type long, so %ld is correct for that field.  But yeah, it does remove -Werror, which is the right thing.

I have not tried to build pkt-gen outside of the port.
Comment 10 Jose Luis Duran 2023-04-19 17:55:51 UTC
(In reply to Robert Clausecker from comment #9)
> That patch is wrong as tv_nsec is always of type long, so %ld is correct for that field
That was my bad, it should be fixed now.
Comment 11 Jose Luis Duran 2023-04-19 17:57:17 UTC
Created attachment 241592 [details]
Proposed upstream patch to fix compilation on some Tier-2 platforms

If it is not too much to ask, could you try building the port with the attached patch?

I have yet to check the diffs with base...
Comment 12 Robert Clausecker freebsd_committer freebsd_triage 2023-04-19 18:10:57 UTC
Your patch from attachment #241592 [details] builds fine on armv7.

I'll be restoring the original patches with a commit in the next few days.
Please submit your changes upstream so they may be part of a future version of the upstream package.
Comment 13 Jose Luis Duran 2023-04-19 18:23:21 UTC
(In reply to Robert Clausecker from comment #12)
Yes, I'll split it in two: one for the casting and another one (that might not be accepted), for the removals of -Werror.
Thank you!
Comment 14 Jose Luis Duran 2023-04-19 20:29:56 UTC
I'm currently preparing a poudriere setup for testing.

Since it may take longer than you landing patches, I am debating if we should cast to intmax_t instead?

I will attach another possible fix, which I believe is an improvement.
Comment 15 Jose Luis Duran 2023-04-19 20:31:13 UTC
Created attachment 241597 [details]
Proposed upstream patch to fix compilation on 32-bit architectures (intmax_t)

Another alternative, casting to intmax_t.
Comment 16 Jose Luis Duran 2023-04-20 01:31:13 UTC
Created attachment 241604 [details]
Proposed upstream patch to fix compilation on 32-bit architectures (intmax_t)

Use the right commit id.
Comment 17 commit-hook freebsd_committer freebsd_triage 2023-04-20 13:36:24 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=289d41817be3eb14823fb9b550dd0e1186f654b8

commit 289d41817be3eb14823fb9b550dd0e1186f654b8
Author:     Robert Clausecker <fuz@FreeBSD.org>
AuthorDate: 2023-04-19 12:10:57 +0000
Commit:     Robert Clausecker <fuz@FreeBSD.org>
CommitDate: 2023-04-20 11:49:22 +0000

    net/pkt-gen: fix on 32 bit platforms (again)

    Exorcise -Werror for the third time.
    Re-add patches needed to build on 32 bit platforms.
    Please don't remove patches without understanding them first.

    Fixes:          9305cd3f9321bfba95479b2b2a15c7639a790383
    PR:             270440
    See also:       PR 264561, 230623
    Approved by:    portmgr (build fix blanket)

 net/pkt-gen/Makefile                                    |  4 +---
 net/pkt-gen/files/patch-apps_nmreplay_GNUmakefile (new) | 12 ++++++++++++
 net/pkt-gen/files/patch-apps_pkt-gen_GNUmakefile (new)  | 12 ++++++++++++
 net/pkt-gen/files/patch-apps_pkt-gen_pkt-gen.c (new)    | 13 +++++++++++++
 net/pkt-gen/files/patch-libnetmap_GNUmakefile (new)     | 11 +++++++++++
 5 files changed, 49 insertions(+), 3 deletions(-)