From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A109F459AC; Mon, 16 Sep 2024 10:51:30 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 34C714025F; Mon, 16 Sep 2024 10:51:30 +0200 (CEST) Received: from mail-lf1-f45.google.com (mail-lf1-f45.google.com [209.85.167.45]) by mails.dpdk.org (Postfix) with ESMTP id 0EBD340041 for ; Mon, 16 Sep 2024 10:51:29 +0200 (CEST) Received: by mail-lf1-f45.google.com with SMTP id 2adb3069b0e04-53653682246so4690034e87.1 for ; Mon, 16 Sep 2024 01:51:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1726476688; x=1727081488; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=pZrLFivPluhsBcDV7N9kMHaGVSgfjITlH6PbovHycr4=; b=lA60RsuGV3Erykh/OvskZjxc87tK8GCpsiyrNtOIxewHIzeR4nzavA3/aEIFwbA+ph wi1C2G/yx9BrHOE9lO7ei25yzv+6GpfGWG3OGqZgCyHBt0xpKqNPqA/oDZqcrCq0ZrBv rmZoOc8wehyr+a64Uvh7EPFmCAg0ApanEHFEkz3v7pGd1yX+1kCp0EL9Z/F1L8cZ7IsV UNJ2fff5Na5z9p1/pQjGB2YoHvAnPmz2srSL4LX7qAu5CEIp5oFY+cpFAQzu5ey52LRa BVT5VYjDUWRnbiJzYeI37khwO7DXQuHhKGv4ukXlHqjjqdImTP2wYidh/F4Q7axCAPtb 2VfQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726476688; x=1727081488; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=pZrLFivPluhsBcDV7N9kMHaGVSgfjITlH6PbovHycr4=; b=v4TJUsYAcPq6Ty1HPLrPVm1ljn9VvcH2W8XuQT0g9VPDzTy2THWih9T9jLzYUGDIVw My7GAF3nubM5NkdCuAx28wxj9g6zAFH7K0pFoqtovKMde/1sl/G1p9y5FJWSDac70xDe m/DqeKIBxnXfVgcBlDgL7KBPZLxVJcYmpeoFxsKVeYsoJQ5XMJmhKLmIQHXMD9tvdY3w szizWFq5nDMkTXApORPcTxEmfecbZUhPzWyeeNfZXZTzdaOd/LrWN8JgkPMCVriZ2pV7 8Kas79W0VYnfwQclirCQ3xSE+cPaJwWPGqzNtGPAag1/S909Pv9/8cIb645vHb68bOA2 WzXg== X-Forwarded-Encrypted: i=1; AJvYcCVv9d4B2k+P0mFpA83nNFf/QFdCJ1h7p9mprWi2mZ54aqQ1TGjlINsS4aC/vQralAGx7Uk=@dpdk.org X-Gm-Message-State: AOJu0YzyfBKXHHH1BWp5XmgJjIoA48so8MStPBhYpMlPL1H4MNBV2FZt UqMpmUVFAfLGcJHsx/WEmfQqC2bogvHOS/AICIRihubSbLrYTM4sIjR0ozorAFo= X-Google-Smtp-Source: AGHT+IEzYDFlNjH8ScmsJWyc9Y1TRRtmG0vGnttLyUoP97S7jiWVs7BkazuDPfIfW5UVYtXL9ZUXKg== X-Received: by 2002:a05:6512:318c:b0:52c:dfa7:9f43 with SMTP id 2adb3069b0e04-53678fcd6abmr6584208e87.34.1726476688022; Mon, 16 Sep 2024 01:51:28 -0700 (PDT) Received: from [192.168.200.22] ([84.245.121.62]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a90612e519esm286316866b.176.2024.09.16.01.51.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 Sep 2024 01:51:27 -0700 (PDT) Message-ID: <80f57b93-eb91-48b1-ada9-d23fd45f8a59@pantheon.tech> Date: Mon, 16 Sep 2024 10:51:25 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v19 5/5] dts: add API doc generation To: Thomas Monjalon Cc: Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com, Luca.Vizzarro@arm.com, npratte@iol.unh.edu, dmarx@iol.unh.edu, alex.chapman@arm.com, dev@dpdk.org, Bruce Richardson References: <20231115133606.42081-1-juraj.linkes@pantheon.tech> <20240821150254.158912-1-juraj.linkes@pantheon.tech> <20240821150254.158912-6-juraj.linkes@pantheon.tech> <13597798.dW097sEU6C@thomas> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <13597798.dW097sEU6C@thomas> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 12. 9. 2024 22:09, Thomas Monjalon wrote: > 21/08/2024 17:02, Juraj Linkeš: >> +if 'dts' in src: >> + os.environ['DTS_BUILD'] = "y" > > That's more precisely "DTS doc build". > I think the variable name DTS_BUILD may be confusing. Ack, I'll rename the variable. > > [...] >> --- /dev/null >> +++ b/buildtools/get-dts-runtime-deps.py >> @@ -0,0 +1,95 @@ >> +#!/usr/bin/env python3 >> +# SPDX-License-Identifier: BSD-3-Clause >> +# Copyright(c) 2024 PANTHEON.tech s.r.o. >> +# > > extra blank line above can be removed > Ack. >> + >> +"""Utilities for DTS dependencies. >> + >> +The module can be used as an executable script, >> +which verifies that the running Python version meets the version requirement of DTS. >> +The script exits with the standard exit codes in this mode (0 is success, 1 is failure). > > Given it is just doing a check by default, > the script name could be "check-dts-requirements". > Ack. >> + >> +The module also contains a function, get_missing_imports, >> +which looks for runtime dependencies in the DTS pyproject.toml file >> +and returns a list of module names used in an import statement (import packages) that are missing. >> +This function is not used when the module is run as a script and is available to be imported. >> +""" > [...] >> + req_deps = _get_dependencies(_DTS_DEP_FILE_PATH) >> + req_deps.pop('python') >> + >> + for req_dep, dep_data in (req_deps | _EXTRA_DEPS).items(): > > Please could you explain somewhere why _EXTRA_DEPS is needed? I'll add this comment above the variable: # The names of packages used in import statements may be different from distribution package names. # We get distribution package names from pyproject.toml. # _EXTRA_DEPS adds those import names which don't match their distribution package name. > > [...] >> +++ b/doc/api/dts/meson.build >> @@ -0,0 +1,31 @@ >> +# SPDX-License-Identifier: BSD-3-Clause >> +# Copyright(c) 2023 PANTHEON.tech s.r.o. >> + >> +sphinx = find_program('sphinx-build', required: get_option('enable_docs')) >> +if not sphinx.found() >> + subdir_done() >> +endif >> + >> +python_ver_satisfied = run_command(get_dts_runtime_deps, check: false).returncode() >> +if python_ver_satisfied != 0 >> + subdir_done() >> +endif > > Looks simple. > So if I have the right Python but some dependencies are missing, > it will still work the same, right? Yes. > I feel the need for dependencies should be explained in the script. From my point of view, the script gets the dependencies and it's up to the caller how they use the list of dependencies. The caller is conf.py and there's a bit of an explanation: # Get missing DTS dependencies. Add path to buildtools to find the get_missing_imports function. And then: # Ignore missing imports from DTS dependencies. So basically get the dependencies so we know what to ignore. But I could add something to the script if this is not enough. > > [...] >> --- a/doc/api/meson.build >> +++ b/doc/api/meson.build >> @@ -1,6 +1,11 @@ >> # SPDX-License-Identifier: BSD-3-Clause >> # Copyright(c) 2018 Luca Boccassi >> >> +# initialize common Doxygen configuration >> +cdata = configuration_data() >> + >> +subdir('dts') > > Why inserting DTS first before generating DPDK API doc? > I wanted to put it before subdir_done(). Maybe we could put subdir('dts') in the else branch and also at the end of the meson.build file. That could be better. > [...] >> # set up common Doxygen configuration >> -cdata = configuration_data() >> cdata.set('VERSION', meson.project_version()) >> cdata.set('API_EXAMPLES', join_paths(dpdk_build_root, 'doc', 'api', 'examples.dox')) >> cdata.set('OUTPUT', join_paths(dpdk_build_root, 'doc', 'api')) >> diff --git a/doc/guides/conf.py b/doc/guides/conf.py >> index 0f7ff5282d..d7f3030838 100644 >> --- a/doc/guides/conf.py >> +++ b/doc/guides/conf.py >> @@ -10,7 +10,7 @@ >> from os.path import basename >> from os.path import dirname >> from os.path import join as path_join >> -from sys import argv, stderr >> +from sys import argv, stderr, path >> >> import configparser >> >> @@ -58,6 +58,48 @@ >> ("tools/devbind", "dpdk-devbind", >> "check device status and bind/unbind them from drivers", "", 8)] >> >> +# DTS API docs additional configuration >> +if environ.get('DTS_BUILD'): >> + extensions = ['sphinx.ext.napoleon', 'sphinx.ext.autodoc', 'sphinx.ext.intersphinx'] >> + # Napoleon enables the Google format of Python doscstrings. >> + napoleon_numpy_docstring = False >> + napoleon_attr_annotations = True >> + napoleon_preprocess_types = True >> + >> + # Autodoc pulls documentation from code. >> + autodoc_default_options = { >> + 'members': True, >> + 'member-order': 'bysource', >> + 'show-inheritance': True, >> + } >> + autodoc_class_signature = 'separated' >> + autodoc_typehints = 'both' >> + autodoc_typehints_format = 'short' >> + autodoc_typehints_description_target = 'documented' >> + >> + # Intersphinx allows linking to external projects, such as Python docs. >> + intersphinx_mapping = {'python': ('https://docs.python.org/3', None)} > > I'm not sure about the need for this intersphinx. It's not stricly needed, but it produces better documentation, with links to Python docs for classes and other things found there. For example: :class:`~argparse.Action` in a docstring will link to https://docs.python.org/3/library/argparse.html#argparse.Action > >> + >> + # DTS docstring options. >> + add_module_names = False >> + toc_object_entries = True >> + toc_object_entries_show_parents = 'hide' >> + # DTS Sidebar config. >> + html_theme_options = { >> + 'collapse_navigation': False, >> + 'navigation_depth': -1, # unlimited depth >> + } >> + >> + # Add path to DTS sources so that Sphinx can find them. >> + dpdk_root = dirname(dirname(dirname(__file__))) >> + path.append(path_join(dpdk_root, 'dts')) >> + >> + # Get missing DTS dependencies. Add path to buildtools to find the get_missing_imports function. >> + path.append(path_join(dpdk_root, 'buildtools')) >> + import importlib >> + # Ignore missing imports from DTS dependencies. >> + autodoc_mock_imports = importlib.import_module('get-dts-runtime-deps').get_missing_imports() > [...] >> +the corresponding changes must be made to DTS api doc sources in ``doc/api/dts``. > > api -> API > Ack. > > Except minor corrections and explanations, it looks good. > You can add my ack to the final version. > > Acked-by: Thomas Monjalon > >