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

doc: Document binary blobs #47678

Merged
merged 1 commit into from Aug 12, 2022

Conversation

carlescufi
Copy link
Member

As approved by the TSC on February 9th, 2022:
https://docs.google.com/document/d/1_sFM1BTLHrwRA9pbNoD7XPHE3OuqXT0P04AlAS2dvTk/edit#heading=h.y6xurk7xxg5a

Original document that was voted on:
https://docs.google.com/document/d/1heqcv7dzGvM5rA9xpTMW3kyKJLpZsl2Gjsmr5eqBje8/edit#

This initial patch documents most of the agreed upon decisions, with
additional information and infrastructure to come next.

Signed-off-by: Carles Cufi carles.cufi@nordicsemi.no

Comment on lines 154 to 160
To mitigate this risk, the scope of upstream library blobs will be limited. The
project maintainers must define an open source test suite that an upstream
target must be able to pass using only open source software included in the
mainline distribution and its modules. The scope of this test suite may grow
over time, but the goal is to specify tests for a minimal feature set which must
be supported via open source software for any target with upstream Zephyr
support.
Copy link
Member Author

Choose a reason for hiding this comment

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

In this Pull Request we should decide on this particular point, or at least on an initial version of it.

I suggest we require all platforms to be able to run the tests/kernel suite without the use of library binary blobs (requiring a fw image is OK).

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a good starting point. Perhaps we can have a special "opensource" tag in the testcase.yml files to allow this to be easily ran using twister.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will start with:

  • samples/philosophers
  • tests/kernel

@carlescufi carlescufi added the TSC Topics that need TSC discussion label Jul 12, 2022
@carlescufi carlescufi removed the request for review from utzig July 12, 2022 11:19
@zephyrbot zephyrbot requested a review from utzig July 12, 2022 11:21
@carlescufi carlescufi removed the request for review from utzig July 12, 2022 11:28
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Thanks @carlescufi, much clearer (to me) than the earlier work documents.

doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
Toolchain requirements
----------------------

Library blobs must be in a data format which is compatible with a toolchain
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Library blobs must be in a data format which is compatible with a toolchain
Library blobs must be in a data format which is compatible with and can be linked by a toolchain

doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
Comment on lines 154 to 160
To mitigate this risk, the scope of upstream library blobs will be limited. The
project maintainers must define an open source test suite that an upstream
target must be able to pass using only open source software included in the
mainline distribution and its modules. The scope of this test suite may grow
over time, but the goal is to specify tests for a minimal feature set which must
be supported via open source software for any target with upstream Zephyr
support.
Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a good starting point. Perhaps we can have a special "opensource" tag in the testcase.yml files to allow this to be easily ran using twister.

doc/contribute/bin_blobs.rst Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
@mbolivar-nordic mbolivar-nordic added the DNM This PR should not be merged (Do Not Merge) label Jul 20, 2022
@mbolivar-nordic
Copy link
Contributor

Adding DNM for now. As author of the original Google doc, I would like to take some time to make sure this matches the document we voted on carefully.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Overall I think it's a good starting out point; thanks!

I've got a mix of forward-looking and short-term comments in my review. Hopefully it is clear which is which. If not please LMK. Thanks again for translating the google doc over to rst.

doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
@@ -0,0 +1,229 @@
.. _bin-blobs:
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't really tell who this document is for. It's trying to do too many things to live in the contribution guide in my opinion. If not now, then eventually I think we need to tease this apart a bit more.

Given that it's in the contribution guide, I guess the intended audience is people who want to submit blobs.

If that's the case, I think that the language could be tightened up a bit and reworked into more of a "here is how to submit blobs", "here are the things you can do", "here are the things you cannot do", "here are the things you must do".

Right now the language is kind of a mix between things like this:

Zephyr supports downloading and using third-party binary blobs via its built-in mechanisms, with some important caveats, described in the following sections.

which reads like part of a user guide for how to use binary blobs, rather than a contributor guide for how to submit them, and stuff like this:

It is up to the vendor to specify the license as part of the blob submission process.

Which is language I would expect to see in a contribution guide for blobs, and things like this:

The Zephyr Project is not expected to be responsible for the maintenance and
support of contributed binary blobs. As a consequence, at the discretion of the
Zephyr Project release team, and on a case-by-case basis:

  • GitHub issues reported on the zephyr repository tracker that require use of
    blobs to reproduce may not be treated as bugs

Which I would expect somewhere under "Project and Governance" in our documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same feeling when writing it. But I looked at our current documentation for external components and got some inspiration from the layout and contents in there. I would define this page as "the ultimate guide to binary blobs in Zephyr, for interested users and vendors alike". I don't necessarily think this is a bad thing: it combines all the info related to binary blobs and Zephyr in one single page, just like the external contributions does. However, some of this may end up in the modules page in the future, especially when I submit the next PR that contains the actual command and its documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would define this page as "the ultimate guide to binary blobs in Zephyr, for interested users and vendors alike".

That is fine, but then I suggest placing it under doc/develop instead of doc/contribute, since the latter is for documentation regarding contributing to the project, while the former is a more general place for information about how to develop with zephyr.


Tainting will be communicated to the user in the following manners:

- A Kconfig option ``TAINT_BLOBS`` will be set to ``true``
Copy link
Contributor

Choose a reason for hiding this comment

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

y, not true

doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
doc/contribute/bin_blobs.rst Show resolved Hide resolved
doc/contribute/bin_blobs.rst Show resolved Hide resolved
doc/contribute/bin_blobs.rst Outdated Show resolved Hide resolved
@carlescufi carlescufi added TSC Topics that need TSC discussion and removed DNM This PR should not be merged (Do Not Merge) TSC Topics that need TSC discussion labels Aug 10, 2022
@carlescufi
Copy link
Member Author

@henrikbrixandersen @mbolivar-nordic @rettichschnidi @nashif please take another look

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

2 things left for me:

  • I think this belongs under develop/, not contribute/, given the definition of the scope of the document
  • one typo

Tainting will be communicated to the user in the following manners:

- One or more Kconfig options ``TAINT_BLOBS_*`` will be set to ``y``
- The Zephyr build system, during its configuration phase, will issue a warning
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing period at end of sentence in this latest update.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@carlescufi
Copy link
Member Author

carlescufi commented Aug 12, 2022

  • I think this belongs under develop/, not contribute/, given the definition of the scope of the document

I disagree because this is analogous to the external components page in scope in my opinion. Which means we should move both?

It'd be great to get more input on this subject because I do agree with @mbolivar-nordic that it is not completely clear where this belongs. @nashif @henrikbrixandersen @erwango @galak @MaureenHelm any opinions?

As approved by the TSC on February 9th, 2022:
https://docs.google.com/document/d/1_sFM1BTLHrwRA9pbNoD7XPHE3OuqXT0P04AlAS2dvTk/edit#heading=h.y6xurk7xxg5a

Original document that was voted on:
https://docs.google.com/document/d/1heqcv7dzGvM5rA9xpTMW3kyKJLpZsl2Gjsmr5eqBje8/edit#

This initial patch documents most of the agreed upon decisions, with
additional information and infrastructure to come next.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
@erwango
Copy link
Member

erwango commented Aug 12, 2022

  • I think this belongs under develop/, not contribute/, given the definition of the scope of the document

I disagree because this is analogous to the external components page in scope in my opinion. Which means we should move both?

It'd be great to get more input on this subject because I do agree with @mbolivar-nordic that it is not completely clear where this belongs. @nashif @henrikbrixandersen @erwango @galak @MaureenHelm any opinions?

If put under develop/, at the minimum it should be split so that an introductory part is put under /contribute. /contribute is the first place I'd look before trying to push new type of contribution.

@carlescufi
Copy link
Member Author

If put under develop/, at the minimum it should be split so that an introductory part is put under /contribute. /contribute is the first place I'd look before trying to push new type of contribution.

Agreed, hence my preference for this to stay in /contribute. I am fine with splitting this up later down the line, so that we move some of the content out of contribute/ and perhaps into develop/modules.

@henrikbrixandersen
Copy link
Member

It'd be great to get more input on this subject because I do agree with @mbolivar-nordic that it is not completely clear where this belongs. @nashif @henrikbrixandersen @erwango @galak @MaureenHelm any opinions?

I think it belongs under /contribute as this is where I would search for information like this myself. I don't have a strong opinion on it, though.

@mbolivar-nordic mbolivar-nordic merged commit 2c301fe into zephyrproject-rtos:main Aug 12, 2022
@mbolivar-nordic
Copy link
Contributor

Given the consensus that at least some part of this belongs in contribute, I will defer to the group :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Documentation TSC Topics that need TSC discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants