Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build system: add list of features for documentation and sanity checking #20366

Merged
merged 5 commits into from
Feb 25, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 9, 2024

Contribution description

This PR adds a features.yaml file as machine readable but easy to edit format that lists, groups, and documents all features ("feature" as in the the build system entity) in RIOT. A small python script generates a Makefile for and markdown file from that.

The Makefile is used to check if all features provided and requested are actually valid names or typos.

The Markdown file is used by Doxygen to generation a documentation page containing all features.

Testing procedure

  1. Try requesting or providing a non-existing feature. A build for an app+board combo that contains a feature provided or requested that does not exist should fail.
    • Beware: This requires make generate-features to be run after touching features.yaml, as the generated Makefile is tracked as source. This avoids running a python script before each and every of the current 141584 build targets (which would result in 4h of additional CPU time per CI build, if each build would add 0.1s of CPU time.)
  2. Check the generated documentation.
    • Here the markdown for consumption by Doxygen is generated as part of make doc, so this would work right away
  3. Check that make static-tests catches when the list of features in makefiles/features_existing.inc.mk is stale (e.g. you updated features.yaml but haven't run make generate-features yet).

Issues/PRs references

@maribu maribu added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 9, 2024
@github-actions github-actions bot added Area: doc Area: Documentation Area: build system Area: Build system Area: tools Area: Supplementary tools Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration labels Feb 9, 2024
@maribu
Copy link
Member Author

maribu commented Feb 9, 2024

This also needs some documentation. The quick guide is:

  1. Edit features.yaml (in the root of the repo)
  2. Run make generate-features (in the root of the repo)
  3. Run make doc (in the root of the repo)

@maribu maribu force-pushed the makefiles/existing_features branch 2 times, most recently from 8c06116 to c5567a2 Compare February 9, 2024 21:48
@riot-ci
Copy link

riot-ci commented Feb 9, 2024

Murdock results

✔️ PASSED

aa356d8 ci: add test that features_existing.inc.mk is up to date

Success Failures Total Runtime
10009 0 10009 09m:38s

Artifacts

Copy link
Member Author

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI comments

doc/doxygen/src/feature_list.md Outdated Show resolved Hide resolved
doc/doxygen/src/feature_list.md Outdated Show resolved Hide resolved
doc/doxygen/src/feature_list.md Outdated Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
@mguetschow
Copy link
Contributor

This also needs some documentation. The quick guide is:

1. Edit `features.yaml` (in the root of the repo)

2. Run `make generate-features` (in the root of the repo)

3. Run `make doc` (in the root of the repo)

Would it make sense to have doc depend on generate-features then? And we probably should document that workflow right in features.yaml to prevent people changing that without running make generate-features afterwards (does YAML even have comments?).

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, that's a big step towards better documentation!

Did you write all the descriptions anew or did you take them from Kconfig files? If the latter is the case, we have to be careful of having several sources of truth in the tree and should converge to one eventually. Probably worth discussing at the VMA next week (and maybe wait with this PR until then?).

Some nit's below.

dist/tools/features_yaml2mx/features_yaml2mx.py Outdated Show resolved Hide resolved
dist/tools/features_yaml2mx/features_yaml2mx.py Outdated Show resolved Hide resolved
dist/tools/features_yaml2mx/features_yaml2mx.py Outdated Show resolved Hide resolved
makefiles/dependency_resolution.inc.mk Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
features.yaml Outdated Show resolved Hide resolved
@maribu maribu force-pushed the makefiles/existing_features branch from 1d04e07 to e894951 Compare February 12, 2024 10:12
@maribu
Copy link
Member Author

maribu commented Feb 12, 2024

This also needs some documentation.

I added the missing documentation now

@maribu
Copy link
Member Author

maribu commented Feb 12, 2024

Did you write all the descriptions anew or did you take them from Kconfig files?

A bit of both. The KConfig files did not have the grouping and since we disabled the feature check in the CI has started to bit-rot.

If the latter is the case, we have to be careful of having several sources of truth in the tree and should converge to one eventually. Probably worth discussing at the VMA next week (and maybe wait with this PR until then?).

We did so last VMA: https://forum.riot-os.org/t/virtual-maintainer-assembly-vma-2023-11/4058/6

There was a bit of misunderstanding whether we drop all of KConfig or just the dependency (e.g. features and modules modeling). But in either case, the KConfig features are just a legacy as of now and this tries to salvage what I can and wire this back up into the CI.

@maribu
Copy link
Member Author

maribu commented Feb 12, 2024

Would it make sense to have doc depend on generate-features then?

I've added calling the script to generate the markdown file to make doc.

Note: The target make generate-features is still there and will no only create the Makefile. The reason I don't like to add this to very build is CI time. I added a test to static-test that will check if features_existing.inc.mk is still up to date and fail if not. That is not super elegant, but it reduced the CI overhead a lot and should just work.

@maribu
Copy link
Member Author

maribu commented Feb 12, 2024

May I squash?

features.yaml Outdated Show resolved Hide resolved
@mguetschow
Copy link
Contributor

If the latter is the case, we have to be careful of having several sources of truth in the tree and should converge to one eventually. Probably worth discussing at the VMA next week (and maybe wait with this PR until then?).

We did so last VMA: https://forum.riot-os.org/t/virtual-maintainer-assembly-vma-2023-11/4058/6

There was a bit of misunderstanding whether we drop all of KConfig or just the dependency (e.g. features and modules modeling). But in either case, the KConfig features are just a legacy as of now and this tries to salvage what I can and wire this back up into the CI.

Okay, if we all agree that features are really part of the to-be-removed information of KConfig (I myself haven't worked enough with KConfig to judge this), then I would opt for removing the information that has been extracted into features.yaml from the KConfig files within the same PR to avoid confusion in the future.

Would it make sense to have doc depend on generate-features then?

I've added calling the script to generate the markdown file to make doc.

Note: The target make generate-features is still there and will no only create the Makefile. The reason I don't like to add this to very build is CI time. I added a test to static-test that will check if features_existing.inc.mk is still up to date and fail if not. That is not super elegant, but it reduced the CI overhead a lot and should just work.

Sounds sensible to me, but I'm far from an expert on our CI to be able to tell if that's a decent workaround.

May I squash?

Yes, from my side.

@maribu maribu force-pushed the makefiles/existing_features branch from 1403063 to 849c40c Compare February 12, 2024 12:38
@maribu maribu added the State: waiting for CI update State: The PR requires an Update to CI to be performed first label Feb 12, 2024
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind waiting until tomorrow so I can take a quick look? If it is urgent then you may dismiss.

@maribu maribu removed the State: waiting for CI update State: The PR requires an Update to CI to be performed first label Feb 12, 2024
@MrKevinWeiss MrKevinWeiss dismissed their stale review February 13, 2024 10:12

I looked at it... Thanks for waiting.

Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As requested in #20393, it would be practical to link from the feature to supported boards. #20395 has the infrastructure for obtaining that information (which is currently also generated in the generate-features target). To be useful, such lists should make use of board metadata (so that an output can be "all STM32 boards" where applicable), and possibly be collapsed (like, listing 3 and an HTML summary that tells how many there are and expands to the full litany). Given the interdependencies, I suggest not doing this here, but getting this merged and then building on it.

This looks good to me from a high-level perspective (haven't gone through every detail yet, trusting one of those two who have will given a more well-founded ACK that I could give). @mguetschow, have your concerns been addressed?

@chrysn
Copy link
Member

chrysn commented Feb 20, 2024

Two small suggestions for the structuring:

  • Let's split word sizes and architectures. For further processing it's helpful if there are categories in which we'd know that their features are usually mutually exclusive. You could cherry-pick 1f01a7d.
  • We could hoist catch-all features in a group out to the next level (with the alternative of introducing another level of nesting), as done in fd44f59. The CPU Grouping tree is one where features are not mutually exclusive (a CPU may have feature cpu_core_cortexm, spu_stm32 and cpu_stm32f4), but as long as those features are always in ordered combinations, there can be an easy to spot deepest feature.

(But that's just for if you choose to apply Kevin's suggestion, which I'm +0 about -- for if you do apply it, you could apply this before that to apply me the manual conflict resolution).

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we agreed on going forward with this approach for feature documentation at today's VMA, I give an ACK on the implementation as is. As for the specific format, maybe you want to consider @MrKevinWeiss proposal to have fixed keys (moving the title to the sub-structure)?

I do think that the now duplicated information in the Kconfig should be removed in a timely fashion, if not in this PR, then in a follow-up. In the latter case, let's open an issue for it to keep it on the list (maybe you can directly give some pointers as to where you've extracted the information from).

@kaspar030
Copy link
Contributor

As for the specific format, maybe you want to consider @MrKevinWeiss proposal to have fixed keys (moving the title to the sub-structure)?

I'd vote for that, too. My main reason would be that it's easier to represent in Rust :). ... laze uses that scheme, too.

@kaspar030
Copy link
Contributor

I'd vote for that, too. My main reason would be that it's easier to represent in Rust :). ... laze uses that scheme, too.

I'd even go further:

groups:
- title: CPU Features
  help: These features are related to CPU capabilities or just used to
        indicated which CPU family is used.
  groups:
    - title: CPU Capabilities
      help: These correspond to features/capabilities provided by certain CPUs
      features:
        - name: generic_cpu_feature
          help: A generic CPU feature that is not specific to any CPU family.

This maps cleanly to sth like:

struct FeatureYaml {
  groups: Vec<Group>,
}

struct Group {
  name: String,
  help: String,
  groups: Vec<Group>,
  features: Vec<Feature>,
}

struct Feature {
 name: String,
 help: String,
}

While not using the yaml map key as name and yaml map value as text gives up the free uniqueness checks, in practice that's trivial to add. And having explicit fields for e.g., "name" and "help" (instead of directly using key&value) allows adding fields.

@kaspar030
Copy link
Contributor

it's easier to represent in Rust

btw, there's a serde yaml schema generator that makes generating a yaml (and json) schema from those structs trivial.

There is no corresponding driver (yet), so this feature is just
confusing.
@maribu maribu force-pushed the makefiles/existing_features branch from 047bd53 to 5550a73 Compare February 23, 2024 14:02
@maribu
Copy link
Member Author

maribu commented Feb 23, 2024

Rebased on top of #20408 to pass the CI, and squashed.

maribu and others added 4 commits February 23, 2024 15:12
Add a python script that allows having a single YAML file as source of
truth for what features are available, and also add documentation such
as help texts and grouping to them.

This utility converts such a YAML file to a Makefile with the contents

```
FEATURES_EXISTING := \
    feature_a \
    feature_b \
    ... \
    features_z \
    #
```

This allows the Makefile based build system to easily check if all
provided features and all requested features do actually exist.

In addition, this also converts the YAML file to a markdown file that
documents the features. This file is then intended to be used by
Doxygen to document the provided features.

Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
Co-authored-by: chrysn <chrysn@fsfe.org>
This lists and documents features in a machine readable form. It is
intended to be used for documentation and the build system.

Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
Co-authored-by: chrysn <chrysn@fsfe.org>
@maribu maribu force-pushed the makefiles/existing_features branch from 5550a73 to aa356d8 Compare February 23, 2024 14:13
@maribu
Copy link
Member Author

maribu commented Feb 23, 2024

Now with the unused import dropped, all should be green

@maribu
Copy link
Member Author

maribu commented Feb 24, 2024

All green, should be get this in?

@chrysn
Copy link
Member

chrysn commented Feb 25, 2024

There's an implementation detail that could yet be improved but could also turn out to be a very deep time sink hole: Instead of (ab)using make with non-file-name targets to regenerate files, we could embrace make and use it to ensure that whenever the generated features_existing.inc.mk is out of date, it gets rebuilt. The reason I think this may be a huge time sink is that that could interact badly with having the file checked in, and the alternative to not having the file checked in is having a post-checkout step that generates it before forking off into thousands of build tasks, which is yet again complicated. So let's not slow things down here with that.

Given there is an ACK and that everything looks discussed and ready, pushing the button.

@chrysn chrysn added this pull request to the merge queue Feb 25, 2024
Merged via the queue into RIOT-OS:master with commit 9e5e665 Feb 25, 2024
25 checks passed
@maribu maribu deleted the makefiles/existing_features branch February 25, 2024 19:44
@maribu
Copy link
Member Author

maribu commented Feb 25, 2024

Yeah! Thx :)

- name: periph_flashpage_pagewise
help: The Flashpage peripheral supports pagewise writing.
- name: periph_flashpage_rwee
help: The Flashpage peripheral is of the Read While Write.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't know what that means 😄

@chrysn
Copy link
Member

chrysn commented Feb 26, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants