| Message ID | dc4bae7a14518fbfff20a0f539df06a5c19b09de.1631783482.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix some issues at documentation | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next | pending | VM_Test |
| bpf/vmtest-bpf-next-PR | pending | PR summary |
| bpf/vmtest-bpf | pending | VM_Test |
| bpf/vmtest-bpf-PR | pending | PR summary |
2021-09-16 11:14 UTC+0200 ~ Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > The file name: Documentation/bpftool-prog.rst > should be, instead: tools/bpf/bpftool/Documentation/bpftool-prog.rst. > > Update its cross-reference accordingly. > > Fixes: a2b5944fb4e0 ("selftests/bpf: Check consistency between bpftool source, doc, completion") > Fixes: ff69c21a85a4 ("tools: bpftool: add documentation") Hi, How is this a fix for the commit that added the documentation in bpftool? > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > tools/testing/selftests/bpf/test_bpftool_synctypes.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/test_bpftool_synctypes.py b/tools/testing/selftests/bpf/test_bpftool_synctypes.py > index be54b7335a76..27a2c369a798 100755 > --- a/tools/testing/selftests/bpf/test_bpftool_synctypes.py > +++ b/tools/testing/selftests/bpf/test_bpftool_synctypes.py > @@ -374,7 +374,7 @@ class ManProgExtractor(ManPageExtractor): > """ > An extractor for bpftool-prog.rst. > """ > - filename = os.path.join(BPFTOOL_DIR, 'Documentation/bpftool-prog.rst') > + filename = os.path.join(BPFTOOL_DIR, 'tools/bpf/bpftool/Documentation/bpftool-prog.rst') > > def get_attach_types(self): > return self.get_rst_list('ATTACH_TYPE') > No I don't believe it should. BPFTOOL_DIR already contains 'tools/bpf/bpftool' and the os.path.join() concatenates the two path fragments. Where is this suggestion coming from? Did you face an issue with the script? Same comment applies for the next two patches. Quentin
Hi Quentin, Em Thu, 16 Sep 2021 10:43:45 +0100 Quentin Monnet <quentin@isovalent.com> escreveu: > 2021-09-16 11:14 UTC+0200 ~ Mauro Carvalho Chehab > <mchehab+huawei@kernel.org> > > The file name: Documentation/bpftool-prog.rst > > should be, instead: tools/bpf/bpftool/Documentation/bpftool-prog.rst. > > > > Update its cross-reference accordingly. > > > > Fixes: a2b5944fb4e0 ("selftests/bpf: Check consistency between bpftool source, doc, completion") > > Fixes: ff69c21a85a4 ("tools: bpftool: add documentation") > > Hi, > How is this a fix for the commit that added the documentation in bpftool? > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > --- > > tools/testing/selftests/bpf/test_bpftool_synctypes.py | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/bpf/test_bpftool_synctypes.py b/tools/testing/selftests/bpf/test_bpftool_synctypes.py > > index be54b7335a76..27a2c369a798 100755 > > --- a/tools/testing/selftests/bpf/test_bpftool_synctypes.py > > +++ b/tools/testing/selftests/bpf/test_bpftool_synctypes.py > > @@ -374,7 +374,7 @@ class ManProgExtractor(ManPageExtractor): > > """ > > An extractor for bpftool-prog.rst. > > """ > > - filename = os.path.join(BPFTOOL_DIR, 'Documentation/bpftool-prog.rst') > > + filename = os.path.join(BPFTOOL_DIR, 'tools/bpf/bpftool/Documentation/bpftool-prog.rst') > > > > def get_attach_types(self): > > return self.get_rst_list('ATTACH_TYPE') > > > > No I don't believe it should. BPFTOOL_DIR already contains > 'tools/bpf/bpftool' and the os.path.join() concatenates the two path > fragments. > > Where is this suggestion coming from? Did you face an issue with the script? No, I didn't face any issues with this script. The suggestion cames from the script at: ./scripts/documentation-file-ref-check which is meant to discover broken doc references. Such script has already a rule to handle stuff under tools/: # Accept relative Documentation patches for tools/ if ($f =~ m/tools/) { my $path = $f; $path =~ s,(.*)/.*,$1,; next if (grep -e, glob("$path/$ref $path/../$ref $path/$fulref")); } but it seems it needs a fixup in order for it to stop reporting issues at test_bpftool_synctypes.py: $ ./scripts/documentation-file-ref-check ... tools/testing/selftests/bpf/test_bpftool_synctypes.py: Documentation/bpftool-prog.rst tools/testing/selftests/bpf/test_bpftool_synctypes.py: Documentation/bpftool-map.rst tools/testing/selftests/bpf/test_bpftool_synctypes.py: Documentation/bpftool-cgroup.rst I'll drop the patches touching it for a next version, probably adding a fix for such script. Thanks, Mauro
2021-09-16 12:49 UTC+0200 ~ Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > Hi Quentin, > > Em Thu, 16 Sep 2021 10:43:45 +0100 > Quentin Monnet <quentin@isovalent.com> escreveu: > >> 2021-09-16 11:14 UTC+0200 ~ Mauro Carvalho Chehab >> <mchehab+huawei@kernel.org> >>> The file name: Documentation/bpftool-prog.rst >>> should be, instead: tools/bpf/bpftool/Documentation/bpftool-prog.rst. >>> >>> Update its cross-reference accordingly. >>> >>> Fixes: a2b5944fb4e0 ("selftests/bpf: Check consistency between bpftool source, doc, completion") >>> Fixes: ff69c21a85a4 ("tools: bpftool: add documentation") >> >> Hi, >> How is this a fix for the commit that added the documentation in bpftool? >> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> >>> --- >>> tools/testing/selftests/bpf/test_bpftool_synctypes.py | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tools/testing/selftests/bpf/test_bpftool_synctypes.py b/tools/testing/selftests/bpf/test_bpftool_synctypes.py >>> index be54b7335a76..27a2c369a798 100755 >>> --- a/tools/testing/selftests/bpf/test_bpftool_synctypes.py >>> +++ b/tools/testing/selftests/bpf/test_bpftool_synctypes.py >>> @@ -374,7 +374,7 @@ class ManProgExtractor(ManPageExtractor): >>> """ >>> An extractor for bpftool-prog.rst. >>> """ >>> - filename = os.path.join(BPFTOOL_DIR, 'Documentation/bpftool-prog.rst') >>> + filename = os.path.join(BPFTOOL_DIR, 'tools/bpf/bpftool/Documentation/bpftool-prog.rst') >>> >>> def get_attach_types(self): >>> return self.get_rst_list('ATTACH_TYPE') >>> >> >> No I don't believe it should. BPFTOOL_DIR already contains >> 'tools/bpf/bpftool' and the os.path.join() concatenates the two path >> fragments. >> >> Where is this suggestion coming from? Did you face an issue with the script? > > No, I didn't face any issues with this script. > > The suggestion cames from the script at: > > ./scripts/documentation-file-ref-check > > which is meant to discover broken doc references. > > Such script has already a rule to handle stuff under tools/: > > # Accept relative Documentation patches for tools/ > if ($f =~ m/tools/) { > my $path = $f; > $path =~ s,(.*)/.*,$1,; > next if (grep -e, glob("$path/$ref $path/../$ref $path/$fulref")); > } > > but it seems it needs a fixup in order for it to stop reporting issues > at test_bpftool_synctypes.py: > > $ ./scripts/documentation-file-ref-check > ... > tools/testing/selftests/bpf/test_bpftool_synctypes.py: Documentation/bpftool-prog.rst > tools/testing/selftests/bpf/test_bpftool_synctypes.py: Documentation/bpftool-map.rst > tools/testing/selftests/bpf/test_bpftool_synctypes.py: Documentation/bpftool-cgroup.rst Oh, I see, thanks for explaining. I didn't know this script would catch the paths in bpftool's test file. > > I'll drop the patches touching it for a next version, probably > adding a fix for such script. > > Thanks, > Mauro > Sounds good to me, thanks a lot! Quentin
Em Thu, 16 Sep 2021 11:57:51 +0100 Quentin Monnet <quentin@isovalent.com> escreveu: > 2021-09-16 12:49 UTC+0200 ~ Mauro Carvalho Chehab > <mchehab+huawei@kernel.org> > > Hi Quentin, > > > > Em Thu, 16 Sep 2021 10:43:45 +0100 > > Quentin Monnet <quentin@isovalent.com> escreveu: > > > >> 2021-09-16 11:14 UTC+0200 ~ Mauro Carvalho Chehab > >> <mchehab+huawei@kernel.org> > >>> The file name: Documentation/bpftool-prog.rst > >>> should be, instead: tools/bpf/bpftool/Documentation/bpftool-prog.rst. > >>> > >>> Update its cross-reference accordingly. > >>> > >>> Fixes: a2b5944fb4e0 ("selftests/bpf: Check consistency between bpftool source, doc, completion") > >>> Fixes: ff69c21a85a4 ("tools: bpftool: add documentation") > >> > >> Hi, > >> How is this a fix for the commit that added the documentation in bpftool? > >> > >>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > >>> --- > >>> tools/testing/selftests/bpf/test_bpftool_synctypes.py | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/tools/testing/selftests/bpf/test_bpftool_synctypes.py b/tools/testing/selftests/bpf/test_bpftool_synctypes.py > >>> index be54b7335a76..27a2c369a798 100755 > >>> --- a/tools/testing/selftests/bpf/test_bpftool_synctypes.py > >>> +++ b/tools/testing/selftests/bpf/test_bpftool_synctypes.py > >>> @@ -374,7 +374,7 @@ class ManProgExtractor(ManPageExtractor): > >>> """ > >>> An extractor for bpftool-prog.rst. > >>> """ > >>> - filename = os.path.join(BPFTOOL_DIR, 'Documentation/bpftool-prog.rst') > >>> + filename = os.path.join(BPFTOOL_DIR, 'tools/bpf/bpftool/Documentation/bpftool-prog.rst') > >>> > >>> def get_attach_types(self): > >>> return self.get_rst_list('ATTACH_TYPE') > >>> > >> > >> No I don't believe it should. BPFTOOL_DIR already contains > >> 'tools/bpf/bpftool' and the os.path.join() concatenates the two path > >> fragments. > >> > >> Where is this suggestion coming from? Did you face an issue with the script? > > > > No, I didn't face any issues with this script. > > > > The suggestion cames from the script at: > > > > ./scripts/documentation-file-ref-check > > > > which is meant to discover broken doc references. > > > > Such script has already a rule to handle stuff under tools/: > > > > # Accept relative Documentation patches for tools/ > > if ($f =~ m/tools/) { > > my $path = $f; > > $path =~ s,(.*)/.*,$1,; > > next if (grep -e, glob("$path/$ref $path/../$ref $path/$fulref")); > > } > > > > but it seems it needs a fixup in order for it to stop reporting issues > > at test_bpftool_synctypes.py: > > > > $ ./scripts/documentation-file-ref-check > > ... > > tools/testing/selftests/bpf/test_bpftool_synctypes.py: Documentation/bpftool-prog.rst > > tools/testing/selftests/bpf/test_bpftool_synctypes.py: Documentation/bpftool-map.rst > > tools/testing/selftests/bpf/test_bpftool_synctypes.py: Documentation/bpftool-cgroup.rst > > Oh, I see, thanks for explaining. I didn't know this script would catch > the paths in bpftool's test file. > > > > > I'll drop the patches touching it for a next version, probably > > adding a fix for such script. > > > > Thanks, > > Mauro > > > > Sounds good to me, thanks a lot! The enclosed patch should do the trick. > Quentin Thanks, Mauro [PATCH] scripts: documentation-file-ref-check: fix bpf selftests path tools/testing/selftests/bpf/test_bpftool_synctypes.py use relative patches on the top of BPFTOOL_DIR: BPFTOOL_DIR = os.path.join(LINUX_ROOT, 'tools/bpf/bpftool') Change the script to automatically convert: testing/selftests/bpf -> bpf/bpftool In order to properly check the files used by such script. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> diff --git a/scripts/documentation-file-ref-check b/scripts/documentation-file-ref-check index 7187ea5e5149..2d91cfe11cd2 100755 --- a/scripts/documentation-file-ref-check +++ b/scripts/documentation-file-ref-check @@ -144,6 +144,7 @@ while (<IN>) { if ($f =~ m/tools/) { my $path = $f; $path =~ s,(.*)/.*,$1,; + $path =~ s,testing/selftests/bpf,bpf/bpftool,; next if (grep -e, glob("$path/$ref $path/../$ref $path/$fulref")); }
2021-09-16 13:30 UTC+0200 ~ Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > [PATCH] scripts: documentation-file-ref-check: fix bpf selftests path > > tools/testing/selftests/bpf/test_bpftool_synctypes.py use > relative patches on the top of BPFTOOL_DIR: > > BPFTOOL_DIR = os.path.join(LINUX_ROOT, 'tools/bpf/bpftool') > > Change the script to automatically convert: > > testing/selftests/bpf -> bpf/bpftool > > In order to properly check the files used by such script. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > diff --git a/scripts/documentation-file-ref-check b/scripts/documentation-file-ref-check > index 7187ea5e5149..2d91cfe11cd2 100755 > --- a/scripts/documentation-file-ref-check > +++ b/scripts/documentation-file-ref-check > @@ -144,6 +144,7 @@ while (<IN>) { > if ($f =~ m/tools/) { > my $path = $f; > $path =~ s,(.*)/.*,$1,; > + $path =~ s,testing/selftests/bpf,bpf/bpftool,; > next if (grep -e, glob("$path/$ref $path/../$ref $path/$fulref")); > } > > > > I tested the patch and it works well on my side. However, this looks a bit fragile to me. There is no particular reason to have testing/selftests/bpf point to bpf/bpftool other than to accommodate the current case, we could imagine other selftest files pointing to other parts of the documentation in the future. I would instead make an exception for test_bpftool_synctypes.py specifically (other selftest files don't usually parse documentation anyway). Alternatively, I would look at excluding lines where the path is used in code (tricky to detect), maybe at least when used with os.path.join(): next if ($ln =~ m,os\.path\.join\([^\,]*\,\s*['"]$fulref,); But I'm not familiar with documentation-file-ref-check in the first place, so these are just my two cents. Thanks, Quentin
diff --git a/tools/testing/selftests/bpf/test_bpftool_synctypes.py b/tools/testing/selftests/bpf/test_bpftool_synctypes.py index be54b7335a76..27a2c369a798 100755 --- a/tools/testing/selftests/bpf/test_bpftool_synctypes.py +++ b/tools/testing/selftests/bpf/test_bpftool_synctypes.py @@ -374,7 +374,7 @@ class ManProgExtractor(ManPageExtractor): """ An extractor for bpftool-prog.rst. """ - filename = os.path.join(BPFTOOL_DIR, 'Documentation/bpftool-prog.rst') + filename = os.path.join(BPFTOOL_DIR, 'tools/bpf/bpftool/Documentation/bpftool-prog.rst') def get_attach_types(self): return self.get_rst_list('ATTACH_TYPE')
The file name: Documentation/bpftool-prog.rst should be, instead: tools/bpf/bpftool/Documentation/bpftool-prog.rst. Update its cross-reference accordingly. Fixes: a2b5944fb4e0 ("selftests/bpf: Check consistency between bpftool source, doc, completion") Fixes: ff69c21a85a4 ("tools: bpftool: add documentation") Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- tools/testing/selftests/bpf/test_bpftool_synctypes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)