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

XTASK: add a way to activate features per chip (docs) #2287

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

playfulFence
Copy link
Contributor

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

close #2195

Testing

Built the documentation

@playfulFence playfulFence added the skip-changelog No changelog modification needed label Oct 7, 2024
xtask/src/lib.rs Outdated
// If we need to activate a feature for documentation build for particular
// package for chip, add a matching by chip/package/both according to the
// existing example.
let rules: Vec<fn(&Chip, &Package) -> Vec<String>> = vec![|chip, pkg| {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point in this being a vector? It's extremely confusing syntax and doesn't need to be runtime changeable at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, the whole original task is more for the future use, when you potentially have to activate more features for different packages. At this point, if we try to simplify it, it's probably easiest to just go back to the original implementation within a feature (?), correct me if I'm wrong.

So we come to the question of how much effort we want to give to such a “convenience” task. The original idea was to change the enum Packages so that its elements are something like this:

EspHal(Option<Map<Chip, Vec<String>>>)

But something like that would not allow to use some derives, among them - ValueEnum. Of course, we could write our own parser instead, but again, we come back to the question of how much effort we want to spend on such work.

Copy link
Contributor

Choose a reason for hiding this comment

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

So how does this PR actually help with the issue it's meant to close? We still only have a hard-coded set of features for a hard-coded set of targets, except now it's hard to actually read what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

probably something as simple as

match (package,chip) {
    (Package::EspHal,Chip::Esp32) => vec!["quad-psram","ci"],
    (Package::EspHal,Chip::Esp32s2) => vec!["quad-psram","ci"],
    (Package::EspHal,Chip::Esp32s3) => vec!["quad-psram","ci],
    (Package::EspHal,_) => vec!["ci"],
    _ => vec![]
}

Would be more readable

Copy link
Contributor Author

@playfulFence playfulFence Oct 10, 2024

Choose a reason for hiding this comment

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

I originally wanted to use something like this too, but then I realized that at some point, as we add features for the documentation build, it's just going to get huge, innit?
Plus, there is no room for generalization, you have to separately, for each chip for each package, configure the necessary chips.

However, after a private conversation, the conclusion is:
Such cases when we want to generalize things are the rare exception - and we don't expect anything else to be added there.

I won't say I completely agree with this, as to me, then the point of this issue and PR is a bit lost, at least from my (!) vision of the situation. However, I could be wrong and apparently I am in the minority 😅

I'll wait for some feedback here and if there isn't any - I'll apply the changes Björn suggested above and we can merge this then.

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

@jessebraham jessebraham added this pull request to the merge queue Oct 14, 2024
Merged via the queue into esp-rs:main with commit 0dc8dcf Oct 14, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XTASK: Add a way to have features per chip for building documentation
4 participants