Update documentation at repo root #493
No reviewers
Labels
No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-Mbed-TLS/TF-PSA-Crypto!493
Loading…
Reference in a new issue
No description provided.
Delete branch "root-documentation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
Address https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/333
PR checklist
Mostly LGTM, but there are small inaccuracies in a few places. I also left some minor optional comments.
@ -10,6 +10,7 @@ More details on all of these points may be found in the sections below.- [Sign-off](#license-and-copyright): all commits must be signed off.- [Tests](#tests): please ensure the PR includes adequate tests.- [Changelog](#documentation): if needed, please provide a changelog entry.- [Backports](#long-term-support-branches): provide a backport if needed (it's fine to wait until the main PR is accepted).At least
BRANCHES.md, and probablyCONTRIBUTING.mdas well, should list Mbed TLS 3.6 as a long-time support branch. It is relevant for backporting bug fixes.@ -29,0 +32,4 @@The project aims to minimise the impact on users upgrading to newer versions of the library and it should not be necessary for a user to make any changes to their own code to work with a newer version of the library. Unless the user has made an active decision to use newer features, a newer generation of the library or a change has been necessary due to a security issue or other significant software defect, no modifications to their own code should be necessary. To achieve this, API compatibility is maintained between different versions of TF-PSA-Crypto on the main development branch and in LTS (Long Term Support) branches, as described in [BRANCHES.md](BRANCHES.md).To minimise such disruption to users, where a change to the interface is required, all changes to the ABI or API, even on the main development branch where new features are added, need to be justifiable by either being a significant enhancement, new feature or bug fix which is best resolved by an interface change. If there is an API change, the contribution, if accepted, will be merged only when there is a major release.Preexisting for 3.6: should we mention submodules here? If you need to change something in a submodule, you'll need to make multiple pull requests. But that may be too much information here.
Also applies in Mbed TLS.
@ -2,38 +2,89 @@ README for TF-PSA-Crypto========================I don't think the section on the PSA API should come before the general overview of TF-PSA-Crypto. It should come after, and quite possibly doesn't belong here.
That's assuming we reference the document at all. It's intended as an architectural specification, not as user documentation.
“implementation of the PSA driver interface” is a really weird way of describing this part, and
psa-driver-interface.mdis not relevant here. At first, when I read this sentence, I didn't understand what this section was about. I would call it something like “configuration of non-functional aspects of built-in cryptographic mechasnisms such as performance/size trade-offs”.“core” is highly specific jargon, and the PSA driver interface specification is not relevant here other than being the one place that uses the word “core”. We should not reference
psa-driver-interface.mdbecause it's been adopted by PSA and we should probably remove our copy. There are currently no options related to driver dispatch. Suggested wording:We may or may not also list client-server separation. That would make the list of topics complete, but client-server separation is experimental and requires a lot of third-party code, so I don't think we should mention it casually.
If we continue linking to the driver interface specification, this should probably be a relative link since it's in the same project.
I think we should remove our copy of the driver interface specification since it's been adopted by PSA, but the document is still in the 1.0 release so we can keep existing references for now.
This should probably be a relative link.
Should we link to version 1.2?
Minor: In version 1.2, PAKE is an extension in a separate document, so I suggest
@ -30,2 +79,4 @@1. Built-in drivers: Options to configure non-functional aspects of built-incryptographic mechanisms such as performance/size trade-offs.The file `include/psa/crypto_config.h` can be edited manually, or in a moreI think the order here looks messy (why aren't PSA mechanisms and non-PSA mechanisms next to each other? Why isn't performance compromises next to mechanism selections? etc.). But it follows the order in
crypto_config.hand I think that's the right thing to do in the readme. I'd like to change the order of sections, but after the release.@ -50,3 +101,4 @@* `apidoc/modules.html` or `apidoc/topics.html`Compiling---------In recent versions of Doxygen,
modules.htmlhas been renamed totopics.html. For Ubuntu, the change came between 22.04 and 24.04. So I think at this time we should mention both files.@ -68,1 +118,4 @@Clang 3.8 and Arm Compiler 6.21. More recent versions should work. Slightlyolder versions may work.* Python 3.8 or later to generate some source files (see below), the testcode, and to generate sample programs.According to https://pkgs.org/search/?q=cmake the version in CentOS 9 is 3.20.2, but the version in Rocky Linux 9 is 3.26.5. I don't know about RHEL 9 (I can't find the official package anywhere public). Given that it's only a matter of “support ecosystem changes and/or fix regressions” I think we could state 3.20.2 just in case.
Minor: you don't need two versions of Python.
Perl is no longer needed for building TF-PSA-Crypto. It's still used in some test scripts, and to generate some files in Mbed TLS.
system-wide or in a virtual environment (which is what pip strongly recommends over
--user).@ -4,17 +4,17 @@ The examples are generally focused on a particular usage case (eg, support fora restricted number of cryptographic mechanisms) and aim at minimizing resourceMinor: it would be nice to do an out-of-tree build, which would illustrate that the path is relative to the source tree, not to the build tree. Also, it would be nice to use an actual example. For example:
P.S. I really like the fact that our build system supports this now!
AFAIK what's in
pre-push.shworks on macOS if you have the right software installed (uncrustify, precise versions of pylint and mypy (which requires an old version of Python), doxygen...). They should work on macOS, FreeBSD, etc. The only GNU requirement is bash, which is widely available on non-GNU OSes (and last I checked, the ancient bash on macOS was good enough forall.sh).Minor: Only if this has actually been set as a git hook, which is not explained inside this file. The paragraph here makes it look like this will happen automatically
I don't think this is a useful set of pre-push checks. I won't block this PR, but I would prefer any work on git hook scripts not to be in an urgent pre-release PR whose purpose is to improve documentation.
I have removed that file.
That's fair enough. I have removed it.
File removed.
@ -2,38 +2,89 @@ README for TF-PSA-Crypto========================I've removed the first paragraph about PSA in general and changed sub-section titles.
@ -2,38 +2,89 @@ README for TF-PSA-Crypto========================I've removed the reference.
@ -2,38 +2,89 @@ README for TF-PSA-Crypto========================I've kept the reference for the time being.
@ -68,1 +118,4 @@Clang 3.8 and Arm Compiler 6.21. More recent versions should work. Slightlyolder versions may work.* Python 3.8 or later to generate some source files (see below), the testcode, and to generate sample programs.Removed
@ -29,0 +32,4 @@The project aims to minimise the impact on users upgrading to newer versions of the library and it should not be necessary for a user to make any changes to their own code to work with a newer version of the library. Unless the user has made an active decision to use newer features, a newer generation of the library or a change has been necessary due to a security issue or other significant software defect, no modifications to their own code should be necessary. To achieve this, API compatibility is maintained between different versions of TF-PSA-Crypto on the main development branch and in LTS (Long Term Support) branches, as described in [BRANCHES.md](BRANCHES.md).To minimise such disruption to users, where a change to the interface is required, all changes to the ABI or API, even on the main development branch where new features are added, need to be justifiable by either being a significant enhancement, new feature or bug fix which is best resolved by an interface change. If there is an API change, the contribution, if accepted, will be merged only when there is a major release.Not added for the time being.
@davidhorstmann-arm @gilles-peskine-arm I believe I have addressed all comments, please have another look.
@ -2,38 +2,89 @@ README for TF-PSA-Crypto========================I still think the order is wrong, but that's not a blocker.
LGTM
I still think the order of sections in
README.mdcould be improved but that's not a blocker.@ -2,38 +2,89 @@ README for TF-PSA-Crypto========================I was not sure about what you meant about the order. I thought your comment was about the presentation of the PSA API in general. Thus where would you move the "PSA Cryptography API" section?
@ -2,38 +2,89 @@ README for TF-PSA-Crypto========================This readme is for TF-PSA-Crypto, which is a library. This library implements a standard, the PSA Crypto API. The readme should first present the product, and then have a section on the API. The presentation of the product should not assume that readers are already familiar with the API.
So I would open with a general overview of the functionality (cryptographic operations, random generation, key store, key parsing and formatting) with a mention that most of the API is the PSA Crypto API, with a link to the section that describes the API.
Then I think practical considerations such as configuration, compiling, etc. should come before the section on the PSA API.
Also, while we're discussing section ordering, I think compilation should come immediately after configuration. Documentation should probably come before those, and example programs should be next to or part of documentation.
But none of that is a release blocker, so I propose to do another pass on the readme after the release.
A few small things, looks good otherwise.
Apologies for the delayed turnaround.
@ -60,37 +112,71 @@ You need the following tools to build the library from the main branch with theprovided CMake files. TF-PSA-Crypto minimum tool version requirements are setNit:
@ -105,8 +191,8 @@ In order to run the tests, enter:Nit:
LGTM
LGTM, thanks!