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

cpp: Add documentation about Pigweed as a solution #62371

Closed
wants to merge 1 commit into from

Conversation

yperess
Copy link
Collaborator

@yperess yperess commented Sep 7, 2023

Include documentation about:

  • some C++ features via Pigweed
  • instructions how to setup Pigweed

This PR addresses some aspects of #53851 by giving guidance but not making Pigweed the 'selected' approach to C++.

Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

this is great, thank you!

doc/develop/getting_started/index.rst Outdated Show resolved Hide resolved
doc/develop/languages/cpp/pigweed.rst Outdated Show resolved Hide resolved
doc/develop/languages/cpp/pigweed.rst Outdated Show resolved Hide resolved
doc/develop/languages/cpp/pigweed.rst Outdated Show resolved Hide resolved
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

This goes against what was discussed in the TSC meeting on 2023-08-30 regarding recommending using pigweed to make up for currently unimplemented C++ features in the main Zephyr code (see MoM https://docs.google.com/document/d/1cMkXkFN-Yje-7zIGZmWMfkrVCju85UNnKDfjng9vEv0/edit#heading=h.8cm9xcy7g2ul).

Adding pigweed references to the "Language Support" section of the main Zephyr documentation leads users to believe that this is the approach recommended by the Zephyr project, which the TSC discussion did not conclude.

@henrikbrixandersen henrikbrixandersen added TSC Topics that need TSC discussion area: C++ labels Sep 7, 2023
@yperess
Copy link
Collaborator Author

yperess commented Sep 7, 2023

This goes against what was discussed in the TSC meeting on 2023-08-30 regarding recommending using pigweed to make up for currently unimplemented C++ features in the main Zephyr code (see MoM https://docs.google.com/document/d/1cMkXkFN-Yje-7zIGZmWMfkrVCju85UNnKDfjng9vEv0/edit#heading=h.8cm9xcy7g2ul).

Adding pigweed references to the "Language Support" section of the main Zephyr documentation leads users to believe that this is the approach recommended by the Zephyr project, which the TSC discussion did not conclude.

I read through all the notes before putting this PR together. I'm happy to discuss further, but we never excluded this from going into documentation. The discussion went around the use of it as a module and the delegation of saying "we're going to make this official". Unfortunately the meetings notes seem to be missing a key detail that I'm recalling which is that we did discuss having documentation for modules that we want to advertise but aren't in the manifest yet. Putting this in the C++ documentation is the most natural place IMO.

Happy to discuss further.

@nashif
Copy link
Member

nashif commented Sep 7, 2023

@yperess the idea was to add such documentation under external modules as being introduced in this PR #61505

@carlescufi
Copy link
Member

@henrikbrixandersen @yperess @nashif
I think a middle-ground may work here. We could add documentation for Pigweed in a separate section (i.e. move pigweed.rst elsewhere which by the way in my opinion is separate from the goal pursued in #61505, which just documents a list of all modules, and does not add specific module documentation) and then link to it from the C++ language support page:

for an alterntative approach to C++ support please see Pigweed

But for that we would need modules top-level section in the doc that could document at least the default modules provided by Zephyr.

@nashif
Copy link
Member

nashif commented Sep 7, 2023

goal pursued in #61505, which just documents a list of all modules, and does not add specific module documentation) and then link to it from the C++ language support page:

that is not correct, see https://github.com/zephyrproject-rtos/zephyr/pull/61505/files#diff-c22a8fff0128e724a948e334206112f304cb2c0a00e4429abade35390dd7d890R130

@yperess
Copy link
Collaborator Author

yperess commented Sep 7, 2023

@yperess the idea was to add such documentation under external modules as being introduced in this PR #61505

Sure, seems easy enough to just relocate the pigweed.rst. I just put it where people would most naturally look for it. We can either:

  • merge this and move it when your PR lands
  • wait and rebase when your PR lands

Personally I would prefer to land and move later just to have less things up in the air at the same time. The current layout (without the optional modules) makes sense to me and even with optional modules I would put a :ref: to the Pigweed docs from the C++ docs. So i guess I'm not seeing the harm in this being an interim solution.

@carlescufi
Copy link
Member

goal pursued in #61505, which just documents a list of all modules, and does not add specific module documentation) and then link to it from the C++ language support page:

that is not correct, see https://github.com/zephyrproject-rtos/zephyr/pull/61505/files#diff-c22a8fff0128e724a948e334206112f304cb2c0a00e4429abade35390dd7d890R130

OK, sorry I misunderstood this. Then I suggest we merge #61505 first and then add the link to it from the C++ language page.

@kartben
Copy link
Collaborator

kartben commented Oct 2, 2023

OK, sorry I misunderstood this. Then I suggest we merge #61505 first and then add the link to it from the C++ language page.

fwiw #61505 has now been merged. cc @yperess

@kartben
Copy link
Collaborator

kartben commented Nov 29, 2023

@yperess did you want to revisit this one?

@yperess
Copy link
Collaborator Author

yperess commented Nov 29, 2023

@yperess did you want to revisit this one?

Yes please, I'll rebase

Include documentation about:
- some C++ features via Pigweed
- instructions how to setup Pigweed

This PR addresses some aspects of zephyrproject-rtos#53851 by giving guidance but
not making Pigweed the 'selected' approach to C++.

Signed-off-by: Yuval Peress <peress@google.com>
Signed-off-by: Keith Short <keithshort@google.com>
@yperess
Copy link
Collaborator Author

yperess commented Dec 19, 2023

No code/doc changes, just a raw rebase. @kartben @nashif , what's the process for this now? Do I need to propose Pigweed as an addition to submanifests/optional.yaml? The original goal here was just to document that the option exists, I worry that adding it to the submanifest and having to request a fork will be a bit too much right now.

@yperess yperess requested a review from kartben December 19, 2023 16:36
@kartben
Copy link
Collaborator

kartben commented Dec 21, 2023

@nashif can you help here? I don't think we have a requirement for optional modules to live as a fork under the zephyrproject-rtos GH org but I am not sure.

@fabiobaltieri
Copy link
Member

@nashif can you help here? I don't think we have a requirement for optional modules to live as a fork under the zephyrproject-rtos GH org but I am not sure.

Well they are fetched in CI, worth having the conversation but it may not be a bad idea to keep doing so.

@nashif
Copy link
Member

nashif commented Dec 21, 2023

@nashif can you help here? I don't think we have a requirement for optional modules to live as a fork under the zephyrproject-rtos GH org but I am not sure.

there is a requirement, ?"optional modules" are not enabled/active by default, but otherwise follow the same requirement as enabled and active modules, external projects are those which do not exist in the manifest and do not need to be in the zephyr project org:

https://docs.zephyrproject.org/latest/contribute/external.html#integration-as-external-modules

@nashif
Copy link
Member

nashif commented Dec 21, 2023

just submitted a PR for the template, meaning the pigweed rst file needs to be added to this structure and rederenced from elsewhere in the docs.

#66860

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Feb 20, 2024
@github-actions github-actions bot closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants