| Message ID | cover.1738020236.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Improve ABI documentation generation | expand |
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes: > Hi Jon/Greg, > > That's the second version of my RFC patches meant to modenize the ABI > parser that I wrote in Perl. I have a couple of minor comments on the individual patches, but overall I do like this direction. It would be nice, though, if the code were a bit more extensively commented. Parts of it get into the "twistly maze of regexes" mode that can be awfully hard to follow. > On this series we have: > > patches 1 to 11: several bug fixes addressing issues at ABI symbols; 1-3 aren't needed - it seems you already upstreamed #2? For the rest, is there any reason to not apply them right away? They just seem like worthwhile fixes. > patch 12: a fix for scripts/documentation-file-ref-check > patches 13-15: create new script with rest and search logic and > minimally integrate with kernel_abi Sphinx extension(phase 1); > patches 16-19: implement phase 2: class integration (phase 2); > patch 20: fix a bug at kernel_abi: the way it splits lines is buggy; > patches 21-24: rewrite kernel_abi logic to make it simpler and more > robust; > patches 25-27: add cross-reference support at automarkup; > patches 28-36: several ABI cleanups to cope with the improvements; > patch 37: implement undefined command; > patch 38: get rid of the old Perl script. > > To make it easier to review/apply, I may end breaking the next version > on a couple of different patchsets. Still it would be nice to have more > people testing it and providing some feedback. I've looked over everything, though with limited depth. My testing hasn't turned up any problems. I've only tested with current Sphinx, have you tried this with the more ancient versions we support? [It's probably time to raise our minimum version again, especially now that current Sphinx has better performance.] I don't see a whole lot of reasons not to apply this set shortly after the merge window; anybody disagree? Thanks, jon
Em Tue, 28 Jan 2025 15:42:00 -0700 Jonathan Corbet <corbet@lwn.net> escreveu: > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes: > > > Hi Jon/Greg, > > > > That's the second version of my RFC patches meant to modenize the ABI > > parser that I wrote in Perl. > > I have a couple of minor comments on the individual patches, but overall > I do like this direction. > > It would be nice, though, if the code were a bit more extensively > commented. Parts of it get into the "twistly maze of regexes" mode that > can be awfully hard to follow. The regex code is indeed complex, but documenting it is not an easy task. Btw, they are (about) the same that the Perl script does. imported also the documentation for there. I did some extra cleanups/optimizations there, though, after checking the results of some expressions. The big issue is that we don't have an uniform way of defining What: expressions. So, each subsystem (and/or author) document it in different ways. There are even some ABI symbols with: $(readlink)/sys/... (I intend to send a patch for those later on) and: What: /sys/something/... What: .../something_else (I guess ".../" means "/sys/something/...", but I can't be sure, as this is on one driver for a hardware I don't have - so, if I send a patch, I may end breaking it) If you want to understand how the whole set of regexes work, you can run: $ ./scripts/get_abi.py -d 16 undefined --dry-run >/dev/null ... [DEBUG] /sys/kernel/mm/damon/admin/kdamonds/\w+/contexts/\w+/schemes/\w+/quotas/goals/\w+/current_value <== /sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/quotas/goals/<G>/current_value [DEBUG] /sys/kernel/mm/damon/admin/kdamonds/\w+/contexts/\w+/schemes/\w+/quotas/goals/\w+/target_metric <== /sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/quotas/goals/<G>/target_metric [DEBUG] /sys/kernel/mm/damon/admin/kdamonds/\w+/contexts/\w+/schemes/\w+/quotas/goals/\w+/target_value <== /sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/quotas/goals/<G>/target_value [DEBUG] /sys/kernel/mm/damon/admin/kdamonds/\w+/contexts/\w+/schemes/\w+/quotas/goals/nr_goals <== /sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/quotas/goals/nr_goals [DEBUG] /sys/kernel/mm/damon/admin/kdamonds/\w+/contexts/\w+/schemes/\w+/quotas/ms <== /sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/quotas/ms [DEBUG] /sys/kernel/mm/damon/admin/kdamonds/\w+/contexts/\w+/schemes/\w+/quotas/reset_interval_ms <== /sys/kernel/mm/ ... This will place at stderr all regular expressions that are currently parsed (they're currently used only for /sys symbols). Yet, instead of spending too much time documenting them, IMO we shold do the do the reverse: use the AbiRegex class to convert "What:" into a new tag (like "Regex:") and use it as much as possible (we'll still need "What:" for some things that aren't devnodes), as, with regular expressions, symbols can be clearly documented. As on python match groups can be named with: (?P<name>...) this could be used to better describe some arguments, e.g. (picking an easy case): What: /sys/module/<MODULENAME>/srcversion could be described, instead, as: Regex: /sys/module/(?P<MODULENAME>[\w\-]+)/srcversion The Kernel_abi extension (actually AbiParser class) can either display it as-is (my personal preference), or even replace: (?P<MODULENAME>[\w\-]+) with: MODULENAME and still output this at html/pdf output as before, e. g.: What: /sys/module/<MODULENAME>/srcversion Yet, doing it on a consistent way. This is easier said than done, as if we do some automatic conversion, subsystem reviewers/maintainers will need to double-check if the converted expressions make sense. > > On this series we have: > > > > patches 1 to 11: several bug fixes addressing issues at ABI symbols; > > 1-3 aren't needed - it seems you already upstreamed #2? > > For the rest, is there any reason to not apply them right away? They > just seem like worthwhile fixes. > > > patch 12: a fix for scripts/documentation-file-ref-check > > patches 13-15: create new script with rest and search logic and > > minimally integrate with kernel_abi Sphinx extension(phase 1); > > patches 16-19: implement phase 2: class integration (phase 2); > > patch 20: fix a bug at kernel_abi: the way it splits lines is buggy; > > patches 21-24: rewrite kernel_abi logic to make it simpler and more > > robust; > > patches 25-27: add cross-reference support at automarkup; > > patches 28-36: several ABI cleanups to cope with the improvements; > > patch 37: implement undefined command; > > patch 38: get rid of the old Perl script. > > > > To make it easier to review/apply, I may end breaking the next version > > on a couple of different patchsets. Still it would be nice to have more > > people testing it and providing some feedback. > > I've looked over everything, though with limited depth. > My testing hasn't turned up any problems. Great! > I've only tested with current Sphinx, > have you tried this with the more ancient versions we support? Not yet, but I double-checked at Sphinx documentation to be sure that I won't be using any newer methods: I just kept using the same Sphinx API as used by other extensions at the Kernel. For instance this loop: def do_parse(self, content, node): with switch_source_input(self.state, content): self.state.nested_parse(content, 0, node, match_titles=1) was changed on Sphinx 7.4[1], and even nested_parse(match_titles=1) is not the recommended code for versions < 7.4, as there is this replacement function: nested_parse_with_titles() Yet, as they're working fine at least up to version 8.1.3, we can keep using the old way. In any case, I'll do a test before sending the final version to see if it works fine with our minimal version. [1] See: https://www.sphinx-doc.org/en/master/extdev/markupapi.html - On a separate discussion, I noticed one potential compatibility issue we may have with future Python versions, due to some ascii texts formatted as unicode. I'll send later a patch fixing them. Additionally, automarkup has backward-compatible code with Python 2.7. Can I send patches dropping 2.7 support from Sphinx extensions? > [It's probably time to raise our minimum version again, especially now > that current Sphinx has better performance.] Agreed. IMO, we should also increase Python's minimal version. > I don't see a whole lot of reasons not to apply this set shortly after > the merge window; anybody disagree? Thanks, Mauro
Em Wed, 29 Jan 2025 02:45:18 +0100 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > > I've only tested with current Sphinx, > > have you tried this with the more ancient versions we support? > > Not yet, but I double-checked at Sphinx documentation to be sure that > I won't be using any newer methods: I just kept using the same Sphinx > API as used by other extensions at the Kernel. Just checked it with Python 3.6 and Sphinx 3.4 on Fedora 41 with: sudo dnf install python3.6.x86_64 python3.6 -m venv Sphinx-3.4 pip install alabaster Sphinx==3.4.3 pyyaml There were some issues related to problems with early f-string support, as reported by Akira. After applying the enclosed patch, it is now working fine. The only drawback is here: - print(f"Defined on file{'s'[:len(files) ^ 1]}:\t{", ".join(files)}") + print("Defined on file(s):\t" + ", ".join(files)) As I removed the file/files auto-plural logic depending on the files array length. Not a big deal. I'll double-check if there's no other diff between old/new version and add the enclosed patch in the end. Thanks, Mauro [PATCH] scripts/get_abi.py: make it backward-compatible with Python 3.6 Despite being introduced on Python 3.6, the original implementation was too limited: it doesn't accept anything but the argument. Even on python 3.10.12, support was still limited, as more complex operations cause SyntaxError: Exception occurred: File ".../linux/Documentation/sphinx/kernel_abi.py", line 48, in <module> from get_abi import AbiParser File ".../linux/scripts/get_abi.py", line 525 msg += f"{part}\n{"-" * len(part)}\n\n" ^ SyntaxError: f-string: expecting '}' Replace f-strings by normal string concatenation when it doesn't work on Python 3.6. Reported-by: Akira Yokosawa <akiyks@gmail.com> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> diff --git a/scripts/get_abi.py b/scripts/get_abi.py index 543bed397c8c..e6e94f721fff 100755 --- a/scripts/get_abi.py +++ b/scripts/get_abi.py @@ -522,7 +522,7 @@ class AbiParser: if cur_part and cur_part != part: part = cur_part - msg += f"{part}\n{"-" * len(part)}\n\n" + msg += part + "\n"+ "-" * len(part) +"\n\n" msg += f".. _{key}:\n\n" @@ -546,7 +546,7 @@ class AbiParser: msg += f"Defined on file :ref:`{base} <{ref[1]}>`\n\n" if wtype == "File": - msg += f"{names[0]}\n{"-" * len(names[0])}\n\n" + msg += names[0] +"\n" + "-" * len(names[0]) +"\n\n" desc = v.get("description") if not desc and wtype != "File": @@ -570,7 +570,8 @@ class AbiParser: users = v.get("users") if users and users.strip(" \t\n"): - msg += f"Users:\n\t{users.strip("\n").replace('\n', '\n\t')}\n\n" + users = users.strip("\n").replace('\n', '\n\t') + msg += f"Users:\n\t{users}\n\n" ln = v.get("line_no", 1) @@ -596,7 +597,9 @@ class AbiParser: elif len(lines) == 1: f.append(f"{fname}:{lines[0]}") else: - f.append(f"{fname} lines {", ".join(str(x) for x in lines)}") + m = fname + "lines " + m += ", ".join(str(x) for x in lines) + f.append(m) self.log.warning("%s is defined %d times: %s", what, len(f), "; ".join(f)) @@ -644,10 +647,11 @@ class AbiParser: if users: print(f"Users:\t\t\t{users}") - print(f"Defined on file{'s'[:len(files) ^ 1]}:\t{", ".join(files)}") + print("Defined on file(s):\t" + ", ".join(files)) if desc: - print(f"\n{desc.strip("\n")}\n") + desc = desc.strip("\n") + print(f"\n{desc}\n") if not found_keys: print(f"Regular expression /{expr}/ not found.")
Em Tue, 28 Jan 2025 15:42:00 -0700 Jonathan Corbet <corbet@lwn.net> escreveu: > [It's probably time to raise our minimum version again, especially now > that current Sphinx has better performance.] Last change was about one year ago, so it sounds fair to also change Sphinx minimal version with about one year gap. What we have currently is: 2.4.4: minimal version 3.4.3: suggested minimal version. Looking at the relevant release dates, we have: Release 2.4.0 (released Feb 09, 2020) Release 2.4.4 (released Mar 05, 2020) Release 3.4.0 (released Dec 20, 2020) Release 3.4.3 (released Jan 08, 2021) So, the ~one year gap is what it takes to raise the bar from 2.4.4 to 3.4.3. In terms of Python, we're currently at 3.5: Python Release date 3.5 2015-09-13 3.6 2016-12-23 3.7 2018-06-27 3.8 2019-10-14 3.9 2020-10-05 3.10 2021-10-04 (according with https://en.wikipedia.org/w/index.php?title=History_of_Python) Python 3.6 is the first one with f-string support, with is something that most Python programmers use those days. So, IMO, that would be the absolute minimal version we should pick. Yet, IMHO, we should aim to be backard-compatible with the tools available up to a certain date (Jan, 2021) - e. g. we'll aim to support at least a 4 years old toolset for documentation build. So, I'm proposing to change the minimal requirements to: - Sphinx 3.4.3; - Python 3.9 By setting Sphinx minimal version to 3.4.3, we can get rid of all Sphinx backward-compatible code. I have already patches with such changes for it on the top of this RFC. Will send it shortly as RFC, aiming to send the final version after -rc1. Thanks, Mauro
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes: > So, I'm proposing to change the minimal requirements to: > - Sphinx 3.4.3; > - Python 3.9 > > By setting Sphinx minimal version to 3.4.3, we can get rid of all > Sphinx backward-compatible code. That's certainly a nice thought. With regard to Python ... are all reasonable distributions at 3.9 at least? CentOS 9 seems to be there, and Debian beyond it. So probably that is a reasonable floor to set? jon
Em Wed, 29 Jan 2025 08:58:13 -0700 Jonathan Corbet <corbet@lwn.net> escreveu: > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes: > > > So, I'm proposing to change the minimal requirements to: > > - Sphinx 3.4.3; > > - Python 3.9 > > > > By setting Sphinx minimal version to 3.4.3, we can get rid of all > > Sphinx backward-compatible code. > > That's certainly a nice thought. > > With regard to Python ... are all reasonable distributions at 3.9 at > least? CentOS 9 seems to be there, and Debian beyond it. So probably > that is a reasonable floor to set? I didn't check, but those are the current minimal versions above 3.5 for what we have at the Kernel tree[1]: !2, 3.10 tools/net/sunrpc/xdrgen/generators/__init__.py !2, 3.10 tools/net/sunrpc/xdrgen/generators/program.py !2, 3.10 tools/net/sunrpc/xdrgen/subcmds/source.py !2, 3.10 tools/net/sunrpc/xdrgen/xdr_ast.py !2, 3.10 tools/power/cpupower/bindings/python/test_raw_pylibcpupower.py !2, 3.9 tools/testing/selftests/net/rds/test.py !2, 3.9 tools/net/ynl/ethtool.py !2, 3.9 tools/net/ynl/cli.py !2, 3.9 scripts/checktransupdate.py !2, 3.8 tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py !2, 3.8 tools/testing/selftests/hid/tests/base.py !2, 3.7 tools/testing/selftests/turbostat/smi_aperf_mperf.py !2, 3.7 tools/testing/selftests/turbostat/defcolumns.py !2, 3.7 tools/testing/selftests/turbostat/added_perf_counters.py !2, 3.7 tools/testing/selftests/hid/tests/conftest.py !2, 3.7 tools/testing/kunit/qemu_config.py !2, 3.7 tools/testing/kunit/kunit_tool_test.py !2, 3.7 tools/testing/kunit/kunit.py !2, 3.7 tools/testing/kunit/kunit_parser.py !2, 3.7 tools/testing/kunit/kunit_kernel.py !2, 3.7 tools/testing/kunit/kunit_json.py !2, 3.7 tools/testing/kunit/kunit_config.py !2, 3.7 tools/perf/scripts/python/gecko.py !2, 3.7 scripts/rust_is_available_test.py !2, 3.7 scripts/bpf_doc.py !2, 3.6 tools/writeback/wb_monitor.py !2, 3.6 tools/workqueue/wq_monitor.py !2, 3.6 tools/workqueue/wq_dump.py !2, 3.6 tools/usb/p9_fwd.py !2, 3.6 tools/tracing/rtla/sample/timerlat_load.py !2, 3.6 tools/testing/selftests/net/openvswitch/ovs-dpctl.py !2, 3.6 tools/testing/selftests/net/nl_netdev.py !2, 3.6 tools/testing/selftests/net/lib/py/ynl.py !2, 3.6 tools/testing/selftests/net/lib/py/utils.py !2, 3.6 tools/testing/selftests/net/lib/py/nsim.py !2, 3.6 tools/testing/selftests/net/lib/py/netns.py !2, 3.6 tools/testing/selftests/net/lib/py/ksft.py !2, 3.6 tools/testing/selftests/kselftest/ksft.py !2, 3.6 tools/testing/selftests/hid/tests/test_tablet.py !2, 3.6 tools/testing/selftests/hid/tests/test_sony.py !2, 3.6 tools/testing/selftests/hid/tests/test_multitouch.py !2, 3.6 tools/testing/selftests/hid/tests/test_mouse.py !2, 3.6 tools/testing/selftests/hid/tests/base_gamepad.py !2, 3.6 tools/testing/selftests/hid/tests/base_device.py !2, 3.6 tools/testing/selftests/drivers/net/stats.py !2, 3.6 tools/testing/selftests/drivers/net/shaper.py !2, 3.6 tools/testing/selftests/drivers/net/queues.py !2, 3.6 tools/testing/selftests/drivers/net/ping.py !2, 3.6 tools/testing/selftests/drivers/net/lib/py/remote_ssh.py !2, 3.6 tools/testing/selftests/drivers/net/lib/py/load.py !2, 3.6 tools/testing/selftests/drivers/net/lib/py/__init__.py !2, 3.6 tools/testing/selftests/drivers/net/lib/py/env.py !2, 3.6 tools/testing/selftests/drivers/net/hw/rss_ctx.py !2, 3.6 tools/testing/selftests/drivers/net/hw/pp_alloc_fail.py !2, 3.6 tools/testing/selftests/drivers/net/hw/nic_performance.py !2, 3.6 tools/testing/selftests/drivers/net/hw/nic_link_layer.py !2, 3.6 tools/testing/selftests/drivers/net/hw/lib/py/linkconfig.py !2, 3.6 tools/testing/selftests/drivers/net/hw/lib/py/__init__.py !2, 3.6 tools/testing/selftests/drivers/net/hw/devmem.py !2, 3.6 tools/testing/selftests/drivers/net/hw/devlink_port_split.py !2, 3.6 tools/testing/selftests/drivers/net/hw/csum.py !2, 3.6 tools/testing/selftests/devices/probe/test_discoverable_devices.py !2, 3.6 tools/testing/selftests/bpf/test_bpftool_synctypes.py !2, 3.6 tools/testing/selftests/bpf/generate_udp_fragments.py !2, 3.6 tools/testing/kunit/run_checks.py !2, 3.6 tools/testing/kunit/kunit_printer.py !2, 3.6 tools/sched_ext/scx_show_state.py !2, 3.6 tools/perf/tests/shell/lib/perf_metric_validation.py !2, 3.6 tools/perf/tests/shell/lib/perf_json_output_lint.py !2, 3.6 tools/perf/scripts/python/parallel-perf.py !2, 3.6 tools/perf/scripts/python/flamegraph.py !2, 3.6 tools/perf/scripts/python/arm-cs-trace-disasm.py !2, 3.6 tools/perf/pmu-events/models.py !2, 3.6 tools/perf/pmu-events/metric_test.py !2, 3.6 tools/perf/pmu-events/metric.py !2, 3.6 tools/perf/pmu-events/jevents.py !2, 3.6 tools/net/ynl/ynl-gen-rst.py !2, 3.6 tools/net/ynl/ynl-gen-c.py !2, 3.6 tools/net/ynl/lib/ynl.py !2, 3.6 tools/net/ynl/lib/nlspec.py !2, 3.6 tools/crypto/tcrypt/tcrypt_speed_compare.py !2, 3.6 tools/cgroup/iocost_monitor.py !2, 3.6 tools/cgroup/iocost_coef_gen.py !2, 3.6 scripts/make_fit.py !2, 3.6 scripts/macro_checker.py !2, 3.6 scripts/get_abi.py !2, 3.6 scripts/generate_rust_analyzer.py !2, 3.6 scripts/gdb/linux/timerlist.py !2, 3.6 scripts/gdb/linux/pgtable.py !2, 3.6 scripts/clang-tools/run-clang-tools.py !2, 3.6 Documentation/sphinx/automarkup.py [1] Checked with: vermin -v $(git ls-files *.py) Please notice that vermin is not perfect: my script passed as version 3.6 because the f-string check there didn't verify f-string improvements over time. Still, it is a quick way to check that our current minimal version is not aligned with reality. Btw, vermin explains what is requiring more at the scripts. For instance: $ vermin -vv scripts/checktransupdate.py ... !2, 3.9 /new_devel/v4l/docs/scripts/checktransupdate.py 'argparse' module requires 2.7, 3.2 'argparse.BooleanOptionalAction' member requires !2, 3.9 'datetime' module requires 2.3, 3.0 'datetime.datetime.strptime' member requires 2.5, 3.0 'logging' module requires 2.3, 3.0 'logging.StreamHandler' member requires 2.6, 3.0 'os.path.relpath' member requires 2.6, 3.0 f-strings require !2, 3.6 Thanks, Mauro