rpmlint:
ifcfg-devname.x86_64: W: spelling-error Summary(en_US) Udev -> Dude
ifcfg-devname.x86_64: W: spelling-error Summary(en_US) cmdline -> decline
ifcfg-devname.x86_64: E: summary-too-long C Udev helper utility that provides network interface naming using kernel cmdline and ifcfg configuration

I think rpmlint is right that Summary is too long. Maybe "Udev helper to name interfaces in kernel cmdline and ifconfig files"?

Since this is using rust2rpm template, there isn't much to check.
+ package name is OK
+ license is acceptable for Fedora (GPLv3)
+ license is specified correctly
+ latest version
+ R/P/BR look OK
+ fedora-review and rpmlint find no issues, except as noted above

The only question I have is how this relates to systemd-network-generator.
In particular, s-n-g understands ifname=, and it would be awkward if both utilities tried to
handle it.

Thank you Zbigniew for your review, I'll update the description.

Regarding your question, from what I saw in docs, systemd-network-generator does a slightly different thing than ifcfg-devname in my opinion. [1]
The main goal of ifcfg-devname is to replace the rename_device binary from initscripts. [2]

As input it gets the kernel name of the network device and then it looks at kernel-cmdline (ifname=) and ifcfg files (DEVICE=) to look for a new name, which is then returned on std output.

[1] - https://www.freedesktop.org/software/systemd/man/systemd-network-generator.service.html
[2] - https://github.com/fedora-sysv/initscripts/blob/master/src/rename_device.c

I updated summary, please have a look.

> The main goal of ifcfg-devname is to replace the rename_device binary from initscripts. [2]

OK, so how exactly will ifcfg-devname be used?

It will replace rename_device place in Udev rule 60-net.rules provided by initscripts. [1]

> Something like this:
> ACTION=="add", SUBSYSTEM=="net", DRIVERS=="?*", ATTR{type}=="1", PROGRAM="/usr/bin/ifcfg-devname", RESULT=="?*", NAME="$result"

[1] - https://github.com/fedora-sysv/initscripts/blob/89714f93e0831d8b8b64ff753249ceea007bee5e/usr/lib/udev/rules.d/60-net.rules

Hmm, so I don't think this makes sense. We have a generator that generates appropriate .link files to name
devices. This makes the udev rule partially obsolete, i.e. it shouldn't handle ifname=, it that is
already handled by something else that is unconditionally installed. Since we're switching to a new
implementation here, now is a good time to clean up duplications.

So, you are suggesting to drop logic that handles kernel-cmdline ifname= ?

So ifcfg-devname would only handle only ifcfg DEVICE= ?

Note that since this package has a subpackage that ships a binary, you'll need to specify the license for that subpackage separately.
Inheriting "License: GPLv3" from the source package is certainly not correct, since lots of the crate's dependencies are "ASL 2.0 or MIT" or "MIT" only.

(In reply to Jan Macku from comment #7)
> So, you are suggesting to drop logic that handles kernel-cmdline ifname= ?
> 
> So ifcfg-devname would only handle only ifcfg DEVICE= ?

That's my guess, but I'm not an expert on any of this. I'll ask Yu to take a look.

(In reply to Fabio Valentini from comment #8)
> Note that since this package has a subpackage that ships a binary, you'll
> need to specify the license for that subpackage separately.
> Inheriting "License: GPLv3" from the source package is certainly not
> correct, since lots of the crate's dependencies are "ASL 2.0 or MIT" or
> "MIT" only.

Are you sure? MIT + GPLv3 is effectively GPLv3, and License specifies the
effective license [https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_field].

> Are you sure? MIT + GPLv3 is effectively GPLv3, and License specifies the
> effective license [https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_field].

I have not checked the entire dependency tree, just the direct dependencies.
But whether the resulting "effective license" is still GPLv3 or not, my point is that this needs to be *checked* and *documented*.
You can use the following script in a mock shell, after doing a mock build with "--without check" to list all crate licenses in the buildroot:

for i in $(rpm -qa | grep "rust-.*-devel"); do
   rpm -q $i --qf "%{LICENSE}\n";
done | sort | uniq

(Side note: For Rust packages, we actually usually don't even collapse things like "GPLv3 and MIT" into "GPLv3". Speaking for myself, I don't want to get involved in the intricacies of license compatibility matrices, so I just specify the non-collapsed versions explicitly.)

IIUC,
- both commands rename_device and ifcfg-devname handle ifcfg files and extract matching interface name.
- if a matching ifcfg file exists, then interface renaming through .link files needs to be disabled.
- so, 60-net.rules file set NAME=.

That's OK, as it does not conflict any other tools unless a user creates ifcfg files explicitly.

However,
- the commands also handle 'ifname=' kernel command line option.

That's problematic and confuses users, even if rename_device previously did so. 

As already pointed out by Zbigniew in the above, systemd-network-generator produces .link files
from 'ifname=' and other command line options. If ifcfg-devname (or rename_device) is installed, 
then systemd-network-generator.service does not work as expected, even if the service is enabled
and no ifcfg files exist.
At least, it makes harder to debug if an issue related to 'ifname=' handling occurs.

We (unfortunately) still support ifcfg files, but that is mostly compatibility reason, I think.
So, functionalities not related to ifcfg files should be dropped from the code.

(In reply to Fabio Valentini from comment #10)
> > Are you sure? MIT + GPLv3 is effectively GPLv3, and License specifies the
> > effective license [https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_field].
> 
> I have not checked the entire dependency tree, just the direct dependencies.
> But whether the resulting "effective license" is still GPLv3 or not, my
> point is that this needs to be *checked* and *documented*.

I agree only partially. Yes, the maintainer should check the licensing, but we can't really
require the maintainer to do a full license review. In my reviews I check two things:
- is the *declared* license acceptable for Fedora
- does this declared license *match* the license files and/or headers in source files
(i.e. the checks that fedora-review implements)

To be really comprehensive, a full review would include checking the headers and history of
each file, and figuring out if any of the licenses of dependencies might create a licensing
conflict. My understanding is that we trust the upstream authors to do the right thing, i.e.
specify the licensing truthfully and in a way that is compatible with all the dependencies,
also for the resulting binary artifact. Occasionally I'll try to do such a check if something
looks suspicious or strange, but I think it is too much to *require* for every review.

> You can use the following script in a mock shell, after doing a mock build
> with "--without check" to list all crate licenses in the buildroot:
> 
> for i in $(rpm -qa | grep "rust-.*-devel"); do
>    rpm -q $i --qf "%{LICENSE}\n";
> done | sort | uniq
> 
> (Side note: For Rust packages, we actually usually don't even collapse
> things like "GPLv3 and MIT" into "GPLv3". Speaking for myself, I don't want
> to get involved in the intricacies of license compatibility matrices, so I
> just specify the non-collapsed versions explicitly.)

In the particular case of MIT + GPLv3, the resulting license on the binary artifacts is
effectively GPLv3. MIT is generally interpreted to require the MIT software license text
to be preserved along the source, so the requirements of the MIT-licensed deps are fully
satisfied by the way how we distribute rust-*-devel files.

(A somewhat similar case: many packages have build scripts that have different licensing
than the source code. The licensing of the full source can be quite complicated, with a dozen
different licenses… But we don't distribute the build scripts in the binary package, just
a few compiled artfifacts, and the License field describes just the contents of the binary
packages, so the licensing of the build scripts is not relevant to License.)


(In reply to Yu Watanabe from comment #11)
[...]
> So, functionalities not related to ifcfg files should be dropped from the
> code.

Yeah, that matches my suspicions. Strictly speaking, details of behaviour are not part of
review, i.e. the review could be approved even if the new code does the wrong thing in some cases.
But I'd prefer to fix the code first. The package description and summary will also change, and
I think it'd be nicer to improve it before the review is finalized.

Thank you guys for your comments, I agree with dropping "ifname=" functionality from ifcfg-devname. Also I'll have a look at licensing of subpackages.

Once I have a new version I'll submit it here for review.

So ifcfg-devname binary subpackage should be licensed like:

> License:  GPLv3 or ASL 2.0 or MIT

Fabio, am I correct?

(In reply to Jan Macku from comment #14)
> So ifcfg-devname binary subpackage should be licensed like:
> 
> > License:  GPLv3 or ASL 2.0 or MIT
> 
> Fabio, am I correct?

No, that is definitely wrong.
The binary will contain code from projects that are GPLv3 **and** from projects with other licenses (not **or**).

Running the script I provided for you yields this list of licenses for the crate dependencies:

- ASL 2.0 or MIT
- MIT
- MIT or ASL 2.0
- Unlicense or MIT

(plus GPLv3 for the crate itself).

So the license of the binary would be:

- GPLv3 (for code from this crate itself)
- (ASL 2.0 or MIT) and MIT and (MIT or ASL 2.0) and (Unlicense or MIT) for the dependencies

The second item obviously simplifies to "MIT".

So the binary package should have:

License: GPLv3 and MIT

Zbigniew, Fabio,
Could you please review the new spec file and srpm. I updated the licensing of binary and dropped the ifname functionality.

Thanks

I am not familiar with rpm spec file, but the main package requires "initscripts-rename-device", then IIUC, normal install of the main initscripts.rpm also installs rename-device sub rpm.
I do not like the way. As it cannot be simply enable/disable that feature except for installing/uninstalling the rpm, as I already explained in the above in more detail.

But I am not familiar with how we enable new features or drop previous features in Fedora or RHEL.
If this is required for backward compatible, then there is nothing I can say.
But if this not for keeping backward compat, then please drop the requirement.

Relation between initscripts and initscripts-rename-device you described is due to compatibility reasons. Initscripts-rename-device needs to be installed by default (RHEL9) to ensure backwards compatibility since initscripts aren't installed by default.

Also I see you point with the enable/disable approach, but in my opinion using packaging (subpackages) is more efficient, since it requires minimal changes to source code.

I think the dependency in initscripts is fine. initscripts is only installed optionally, and it's OK if it pulls in
some other packages.

--

After the removal of support for ifname= and the other fixes, this seems all OK.

Quoting comment #1 above:

Since this is using rust2rpm template, there isn't much to check.
+ package name is OK
+ license is acceptable for Fedora (GPLv3 and MIT)
+ license is specified correctly
+ latest version
+ R/P/BR look OK
+ fedora-review and rpmlint find no issues, except as noted above

Package is APPROVED.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-ifcfg-devname

Closed via: https://bodhi.fedoraproject.org/updates/FEDORA-2022-f703feb135