Paths

Table of Contentst

net/aquantia-atlantic-kmod: Fix CURRENT build post r353868 (iflib changes)
ClosedPublic
Actions

Authored by nyan_myuji.xyz on Jan 13 2021, 2:49 PM.

Details

Reviewers

lwhsu
philip
koobs

Summary

net/aquantia-atlantic-kmod: Fix CURRENT build post r353868 (iflib changes)

This patch enable the port to build working if_atlantic.ko
on 13-CURRENT, broken since r353868, where `if_multi_apply`
and `if_multiaddr_count` were removed.

PR: 252642
Approved by: koobs (maintainer)
Differential_Revision: D28135
MFH: 2021Q1 (blanket: build fix)

Test Plan

portlint: ???
testport: ??? (poudriere; at least current and 12amd64)
hardware: AQC107 (device id 0x07b1) tested

Diff Detail

Repository

rP FreeBSD ports repository

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 36173
Build 33062: arc lint + arc unit

Event Timeline

nyan_myuji.xyz created this revision.Jan 13 2021, 2:49 PM

Herald added subscribers: Contributor Reviewers (ports), mat. · View Herald TranscriptJan 13 2021, 2:49 PM

nyan_myuji.xyz requested review of this revision.Jan 13 2021, 2:49 PM

Harbormaster completed remote builds in B36170: Diff 82208.Jan 13 2021, 2:49 PM

lwhsu requested changes to this revision.Jan 13 2021, 3:44 PM

lwhsu added a subscriber: lwhsu.

lwhsu added inline comments.

net/aquantia-atlantic-kmod/Makefile
5	I believe `PORTREVISION` is enough here.

This revision now requires changes to proceed.Jan 13 2021, 3:44 PM

nyan_myuji.xyz added inline comments.Jan 13 2021, 5:07 PM

net/aquantia-atlantic-kmod/Makefile
5	The reason why `PORTEPOCH` is used here is to keep the versioning in-sync with the upstream. or should we change `PORTVERSION` to something like `0.0.5p1` ?

nyan_myuji.xyz added a reviewer: philip.Jan 13 2021, 5:08 PM

lwhsu added inline comments.Jan 13 2021, 5:13 PM

net/aquantia-atlantic-kmod/Makefile
5	`PORTREVISION` is exactly what to use to add local patch and keep `PORTVSION` (or `DISTVERSION`) sync with upstream. `PORTEPOCH` should be used carefully because it will stay forever once comes in.

nyan_myuji.xyz added inline comments.Jan 13 2021, 5:18 PM

net/aquantia-atlantic-kmod/Makefile
5	my bad, I thought you mean `PORTVERSION` lmao

Use PORTREVISION instead of PORTEPOCH

Harbormaster completed remote builds in B36173: Diff 82217.Jan 13 2021, 5:21 PM

lwhsu accepted this revision.Jan 13 2021, 5:37 PM

This revision is now accepted and ready to land.Jan 13 2021, 5:37 PM

If the iflib changes have been tested and are known to work, they're probably okay.

Is the extra patch to aq_hw.c intentional? Using random() to generate a MAC address isn't great. Not seeding it makes it even worse.

This revision now requires changes to proceed.Jan 14 2021, 12:50 AM

This needs:

Confirmation of QA: portlint/poudriere on at least CURRENT and 12amd64 (fill in TEST PLAN section)
PR created upstream
Comment added to patch with upstream issue/PR reference and URL

Thank you for the report and patch

koobs edited the summary of this revision. (Show Details)Jan 14 2021, 2:12 AM

koobs added 1 blocking reviewer(s): koobs.

koobs edited the summary of this revision. (Show Details)

koobs added inline comments.Jan 14 2021, 2:15 AM

net/aquantia-atlantic-kmod/Makefile
5	Is this actually required since the build fails to produce a package?

In D28135#628982, @philip wrote:

If the iflib changes have been tested and are known to work, they're probably okay.

Is the extra patch to aq_hw.c intentional? Using random() to generate a MAC address isn't great. Not seeding it makes it even worse.

Yes it is. This is because random has been deprecated and srandom has been removed at https://reviews.freebsd.org/rS356097

nyan_myuji.xyz added inline comments.Jan 14 2021, 2:59 AM

net/aquantia-atlantic-kmod/Makefile
5	I think I don't exactly know what you mean.. On my side poudriere testport passed and is able to produce package for both current and 12.2-release... Portlint also passed.

lwhsu added inline comments.Jan 21 2021, 9:57 AM

net/aquantia-atlantic-kmod/Makefile
5	Yes bump `PORTVERSION` in this case can be discussed more, but since the package content has changed and it would be good for notifying people using -CURRENT. I believe it's a good step.

Use arc4random instead of random

Harbormaster completed remote builds in B36387: Diff 82679.Jan 21 2021, 10:24 AM

Thanks for the changes!

@philip @lwhsu Thank you both for review. Feel free to commit this (pending adding upstream comments to patches).

net/aquantia-atlantic-kmod/files/patch-aq__hw.c
2	Lets add the upstream (PR) description and link here
net/aquantia-atlantic-kmod/files/patch-aq__main.c
2	Lets add the upstream (PR) description and link here

debdrup added a subscriber: debdrup.Jan 28 2021, 3:37 PM

Add link to upstream pr

Harbormaster completed remote builds in B36645: Diff 83208.Feb 1 2021, 11:42 AM

lwhsu accepted this revision.Feb 8 2021, 12:48 PM

Has all the feedback that's been given been addressed?

If so, who's going to have the honour of commiting and pushing this? :)

@koobs Please review and commit this if it's alright?

I'm happy to commit this later today if @koobs doesn't beat me to it.

@philip I may not have time to day, but the change is otherwise approved

This revision is now accepted and ready to land.Feb 9 2021, 12:37 AM

koobs edited the summary of this revision. (Show Details)Feb 9 2021, 12:37 AM

philip mentioned this in rP564758: net/aquantia-atlantic-kmod: Fix build on CURRENT.Feb 9 2021, 2:51 AM

Committed as r564758.

@koobs If you really don't have time, how about passing maintainership to @nyan_myuji.xyz ? He has more changes planned and is willing to take care of this port. I will help him to land more patches.

In D28135#639065, @lwhsu wrote:

@koobs If you really don't have time, how about passing maintainership to @nyan_myuji.xyz ? He has more changes planned and is willing to take care of this port. I will help him to land more patches.

To clarify: I don't currently have time

I have no qualms with a MAINTAINER change however.

In D28135#639079, @koobs wrote:

In D28135#639065, @lwhsu wrote:

@koobs If you really don't have time, how about passing maintainership to @nyan_myuji.xyz ? He has more changes planned and is willing to take care of this port. I will help him to land more patches.

To clarify: I don't currently have time

I have no qualms with a MAINTAINER change however.