On 5/8/25 12:42, Peter Maydell wrote:
On Fri, 28 Feb 2025 at 15:45, Daniel P. Berrangé <berra...@redhat.com> wrote:
Going forward we want all newly created source files to have an
SPDX-License-Identifier tag present.
Initially mandate this for C, Python, Perl, Shell source files,
as well as JSON (QAPI) and Makefiles, while encouraging users
to consider it for other file types.
I noticed recently that this check doesn't actually catch
all cases of new files without an SPDX tag. It looks to
me like the logic is wrong, unless I'm misreading it:
+# All new files should have a SPDX-License-Identifier tag
+ if ($line =~ /^new file mode\s*\d+\s*$/) {
+ if ($expect_spdx) {
+ if ($expect_spdx_file =~
+ /\.(c|h|py|pl|sh|json|inc|Makefile)$/) {
+ # source code files MUST have SPDX license declared
+ ERROR("New file '$expect_spdx_file' requires " .
+ "'SPDX-License-Identifer'");
+ } else {
+ # Other files MAY have SPDX license if appropriate
+ WARNING("Does new file '$expect_spdx_file' need " .
+ "'SPDX-License-Identifer'?");
+ }
+ }
+ $expect_spdx = 1;
+ $expect_spdx_file = undef;
+ } elsif ($expect_spdx) {
+ $expect_spdx_file = $realfile unless
+ defined $expect_spdx_file;
+
+ # SPDX tags may occurr in comments which were
+ # stripped from '$line', so use '$rawline'
+ if ($rawline =~ /SPDX-License-Identifier/) {
+ $expect_spdx = 0;
+ $expect_spdx_file = undef;
+ }
+ }
+
The logic goes:
* when we see a "new file" line, we set expect_spdx to 1
* when we see an SPDX tag we set expect_spdx to 0
* at a later point, if we still have expect_spdx 1 we complain
The problem is that the "later point" here is "we saw another
'new file' line", not "we got to the end of all the parts of
the patch that touch this file". So if the patch adds two
new files then we'll warn for the first one (when we see the
"new file" line for the secand new file), but we won't warn
for a patch which adds only one new file (there's never a second
"new file" line) or for the last new file added in a patch
that adds multiple files.
I'm not sure where the "complain if expect_spdx is 1" check
should go, but I don't think it should be here...
yes. I just made the same observation on this patch :
https://lore.kernel.org/qemu-devel/20250509163645.33050-7-rre...@linux.ibm.com/
Thanks,
C.
