Checkpatch and SPDX

[Yann-lms]

Hi,

In kernel 4.17-rc1, the checkpatch.pl script has been updated. It now checks the SPDX-License-Identifier tag. But it checks that where the kernel awaits the line: the first line of the file. And with // comment style for .c files.

In trusted firmware, as it is not the case, checkpatch displays this warning: Missing or malformed SPDX-License-Identifier tag in line ...

Is it planned to stick to checkpatch and kernel coding style, and then modifiy nearly all the files in TF-A? Or should the line "--ignore SPDX_LICENSE_TAG" be added to .checkpatch.conf?

Thanks and best regards, Yann

[danh-arm]

Thanks for reporting this. It sounds like checkpatch is being overly aggressive here. I don't think SPDX requires the identifier to be the first thing in the file. In fact the guide here gives examples where it is not:

https://spdx.org/sites/cpstandard/files/pages/files/using_spdx_license_list_short_identifiers.pdf

It also sounds strange if // commenting style is required here, when / / is preferred elsewhere.

So at the moment I see no reason to change TF. Maybe you could ask the checkpatch maintainers about this? In the meantime, we'll have to ignore this in TF.

[sbranden]

It’s the defacto linux kernel standard to have to // SPDX comment on the first line.

I’d actually prefer we just fall in line.

On Apr 26, 2018, at 11:04 AM, danh-arm notifications@github.com wrote:

Thanks for reporting this. It sounds like checkpatch is being overly aggressive here. I don't think SPDX requires the identifier to be the first thing in the file. In fact the guide here gives examples where it is not:

https://spdx.org/sites/cpstandard/files/pages/files/using_spdx_license_list_short_identifiers.pdf

It also sounds strange if // commenting style is required here, when / / is preferred elsewhere.

So at the moment I see no reason to change TF. Maybe you could ask the checkpatch maintainers about this? In the meantime, we'll have to ignore this in TF.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[ghost]

It may be what they want to do, but it is most certainly not the standard right now, there are several files that don't do it:

https://github.com/torvalds/linux/blob/0644f186fc9d77bb5bd198369e59fb28927a3692/kernel/async.c#L1

https://github.com/torvalds/linux/blob/0644f186fc9d77bb5bd198369e59fb28927a3692/kernel/audit.c#L1

https://github.com/torvalds/linux/blob/0644f186fc9d77bb5bd198369e59fb28927a3692/arch/arm/mm/alignment.c#L1

https://github.com/torvalds/linux/blob/0644f186fc9d77bb5bd198369e59fb28927a3692/arch/arm64/kernel/cpu_errata.c#L1

I agree with @danh-arm. I'd rather ignore this rule, we've already gone through the painful process of changing the license header in all files, and a script that complains about it doesn't look to me like a good enough reason to do it again. Also, it's not like we are following all the rules of checkpatch, we ignore quite a few of them, and we are generally relaxed about the rest.

EDIT: In their case it may make sense because they have many different license headers so they at least want to keep some consistency with SPDX, specially for source code analysis tools. In this repository we don't have this problem because we make sure that all headers follow the rules (with the exception of external libraries).

[sbranden]

Hi Antonio,

Actually linux format is documented here: https://github.com/torvalds/linux/blob/master/Documentation/process/license-rules.rst

On Apr 27, 2018, at 1:21 AM, Antonio Niño Díaz notifications@github.com wrote:

It may be what they want to do, but it is most certainly not the standard right now:

https://github.com/torvalds/linux/blob/4f7e988e63e336827f4150de48163bed05d653bd/kernel/async.c#L1

https://github.com/torvalds/linux/blob/4f7e988e63e336827f4150de48163bed05d653bd/kernel/irq/cpuhotplug.c#L1

https://github.com/torvalds/linux/blob/4f7e988e63e336827f4150de48163bed05d653bd/kernel/irq/chip.c#L1

https://github.com/torvalds/linux/blob/4f7e988e63e336827f4150de48163bed05d653bd/arch/arm64/kernel/cpu_errata.c#L1

I agree with @danh-arm. I'd rather ignore this rule, we've already gone through the painful process of changing the license header in all files, and a script that complains about it doesn't look to me like a good enough reason to do it again. Also, it's not like we are following all the rules of checkpatch, we ignore quite a few of them, and we are generally relaxed about the rest.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[ghost]

Oh, so we could actually use /* */ style comments. Still, this is mostly helpful for tools that have to deal with huge license headers and they don't know where to find the SPDX tag. Maybe there are even files with more than one tag.

I still think that the resulting license header would look pretty terrible if we did this, and the benefits of doing it are pretty much nonexistent:

/* SPDX-License-Identifier: BSD-3-Clause */
/* Copyright (c) 2015-2017, ARM Limited and Contributors. All rights reserved. */

[danh-arm]

Trusted Firmware moved to using SPDX before the Linux kernel. The Linux kernel guidelines that came after seem reasonable for them but as Antonio says, don't seem to have any value to us. I'd like to avoid a 2nd "touch all" patch in the commit history if possible.

If I don't hear any other comments, I'll merge Antonio's patch to ignore this warning in the next couple of days.

[Yann-lms]

Hi @danh-arm and @antonio-nino-diaz-arm thanks for the answers and patch. I'm OK with that.

BR, Yann

[Yann-lms]

A patch was done to correct the issue. The ticket can be closed.