Mbed-TLS / mbedtls-docs

Version-independent documentation for Mbed TLS
Apache License 2.0
21 stars 28 forks source link

Bring over remaining phabricator content #133

Closed daverodgman closed 9 months ago

daverodgman commented 9 months ago

Port remaining phabricator wiki content:

I've lightly updated the testing notes - there may still be some out-of-date info but I've tried to fix all the major things. I've removed some things that tend to go out of date (e.g. list of supported tool versions).

Non-critical out-of-date things can be fixed in a follow-up and shouldn't scope-creep this PR.

Part of review: please confirm that I haven't missed any useful content from Phabricator https://developer.trustedfirmware.org/w/mbed-tls/

gilles-peskine-arm commented 9 months ago

This resolves the content part of https://github.com/Mbed-TLS/mbedtls/issues/4086 (with a remaining task of hunting down and updating links to the old wiki).

gilles-peskine-arm commented 9 months ago

I've removed some things that tend to go out of date (e.g. list of supported tool versions).

Please do keep the list of tool versions, that is the main information in https://developer.trustedfirmware.org/w/mbed-tls/testing/ci/. It documents the tool versions that we have in our CI, for which otherwise you have to dig down in multiple CI scripts. It's a checklist that team members use to set up their environment when they join.

daverodgman commented 9 months ago

I've removed some things that tend to go out of date (e.g. list of supported tool versions).

Please do keep the list of tool versions, that is the main information in https://developer.trustedfirmware.org/w/mbed-tls/testing/ci/. It documents the tool versions that we have in our CI, for which otherwise you have to dig down in multiple CI scripts. It's a checklist that team members use to set up their environment when they join.

I don't agree, that list was last updated in Sept 2020, and I doubt we would remember to update it in the future. So I don't think it's providing useful information.

mpg commented 9 months ago

It documents the tool versions that we have in our CI, for which otherwise you have to dig down in multiple CI scripts.

Multiple CI scripts really? My understanding is that you only have to look at a single file: https://github.com/Mbed-TLS/mbedtls-test/blob/master/resources/docker_files/ubuntu-22.04/Dockerfile (or whatever version's closest to the distro/version you're using). I has all the info that the table has I think - except as Dave points out, the Dockefiles are always up-to-date while the table isn't. (Case in point: when we added named versions of GnuTLS & OpenSSL on the CI, we didn't update the table.)

I think we should replace the table with a link to one of the Dockerfiles. If there's any additional information needed, we can keep it in addition to the link. For example draw attention to the fact that you need to set up environment variables pointing to GnuTLS & OpenSSL, talk about which versions of Python are supported, because in the Dockerfiles that's mostly implicit (and even then, perhaps we shouldn't directly mention versions of Python here but instead point to the README from the main repo - that's what's most likely to remain up-to-date).

gilles-peskine-arm commented 9 months ago

Multiple CI scripts really? My understanding is that you only have to look at a single file

You have to look at all the docker files. And all.sh to understand how some of the tools are invoked. And scripts/min_requirements.py (from which you deduce scripts/min_requirements.py) which is invoked by Groovy code. Our scripts have a level of complexity where “TRFS, noob” doesn't cut it. Remember, this is information someone needs when they're starting working on the project.

the Dockefiles are always up-to-date while the table isn't.

Sure, that's the curse of documentation. It's still better to have documentation. Documentation in the canonical place rather than a wiki that's being shut down would have a better chance of keeping up-to-date.

mpg commented 9 months ago

You have to look at all the docker files.

Really, why all of them? I thought just the one that's closest to your version would be enough, what exactly am I missing?

And all.sh to understand how some of the tools are invoked.

As I wrote, I think the documentation should point out the need for setting appropriate environment variables - though IMO it should still defer to the Dockerfiles for what variables exactly: just set the same as the Dockerfiles do. Once that's done, is there still a need to look at all.sh? If so, then I agree we should include that in the documentation.

And scripts/min_requirements.py (from which you deduce scripts/min_requirements.py) which is invoked by Groovy code.

Again, I agree the documentation should mention that.

Our scripts have a level of complexity where “TRFS, noob” doesn't cut it. Remember, this is information someone needs when they're starting working on the project.

Sure, I'm all for beginner-friendly documentation. I'm just thinking that when this documentation can reference information that's always up-to-date (like the Dockerfiles) it should do so (with comments on how to use that resource) rather than try duplicating the information. I think "pointer to up-to-date info + comments" (where only the comments might be outdated) beats "duplicated info + comments" (where both the comments and the basic info might be outdated).

daverodgman commented 9 months ago

I agree with Manuel. History has shown that we will not keep it up to date. I also think it's low-value information - most people will never need it and can always ask. A short section to document "here is how you find the minimum tool versions from docker" is fine.

I'm not going to do this in this PR, but if anyone feels motivated, happy to take a separate PR to add such a section.