Hello Marius,

First of all thank you for your work on ‘net-snmp’.

Marius Bakke <mbakke@fastmail.com> writes:

> Oleg Pykhalov <go.wigust@gmail.com> writes:
>
>> * gnu/packages/networking.scm (net-snmp): New variable.
>
> Hello!  This patch reminded me I had an ancient patch for Net-SNMP too,
> but got stuck on a single test failure and forgot all about it.

Oh, unfortunate I didn't find it before #32268.

> I see you've disabled tests altogether which is a neat workaround.
> However I'm hoping we can consolidate our efforts and just disable the
> one (or was it two) tests that are failing.

I did't find tests suite while packaging.  Thank you for pointing that.

> Comments inline, my patch attached at the end.
>
>> ---
>>  gnu/packages/networking.scm | 76 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>
> I chose to add a (gnu packages snmp) module, but that may be overkill
> indeed.

If you think about adding other SNMP specific tools then it's a great
idea.  :-) Maybe a ‘(gnu packages monitoring)’ module could be also in a
consideration?  I vote for ‘(gnu packages networking)’ still.

[…]

> I opted to use the 5.8 pre-release instead.  No strong opinion, but
> since it's a new package and 5.8 is "just around the corner" I think
> that's fine.  WDYT?

Wow, indeed.  Seems I found a 5.8 release.  Maybe we should peek it?
WDYT?  https://sourceforge.net/projects/net-snmp/files/net-snmp/5.8/

> Also note that this packages bundles a copy of OpenSSL, which should be
> purged.
>
>> +    (build-system gnu-build-system)
>> +    (native-inputs
>> +     `(("autoconf" ,autoconf)
>> +       ("automake" ,automake)
>> +       ("libtool" ,libtool)))
>
> Why are these needed?  Because of the patches?

The ‘(invoke "autoreconf" "-vfi")’ requires all those three packages.

>> +    (inputs
>> +     `(("file" ,file)
>> +       ("perl" ,perl)
>> +       ("openssl" ,openssl)))
>
> "file" is an implicit input.  Can you add a comment about why it's
> needed here (I guess it's referenced somewhere?)?

Configuration or complation phases failed without it.  I don't see
‘file’ in your patch.  So, I think I could skip commenting above.  :-)

[…]

>> +    (license license:bsd-3)))
>
> The main license is actually CMU/UCDs "Historic Permission Notice and
> Disclaimer", which is not in Guix.  Do you think it's worth adding, or
> should we simply use a non-copyleft style URI here?

Either way is OK for me, but I vote for non-copyleft style.

Thanks,
Oleg.