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

Add docs for updating external dependencies #1280

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sethmlarson
Copy link
Contributor

@sethmlarson sethmlarson commented Feb 27, 2024

developer-workflow/sbom.rst Outdated Show resolved Hide resolved
developer-workflow/sbom.rst Outdated Show resolved Hide resolved
developer-workflow/sbom.rst Outdated Show resolved Hide resolved
developer-workflow/sbom.rst Outdated Show resolved Hide resolved
Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

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

Thanks for starting this! It's important workflow that we've never properly documented (not just for SBOMs)

Updating external dependencies (cpython-source-deps)
----------------------------------------------------

Dependencies for Windows CPython builds are `stored in a separate repository <https://github.com/python/cpython-source-deps>`_
Copy link
Member

Choose a reason for hiding this comment

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

Some binaries are also stored in https://github.com/python/cpython-bin-deps, though generally they should also have sources in the source-deps repo. Is this distinction important here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do any of the cpython-bin-deps get shipped along with the CPython artifacts? If they're derived from the cpython-source-deps repository I think we should be okay.

Copy link
Member

Choose a reason for hiding this comment

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

I think the only one that isn't derived from cpython-source-deps is vcruntime140.dll, which comes from our repo to make sure we always get the latest one and not whichever GHA build machine we're on.

SBOM tooling in the CPython repository matches these git refs in order to build the :cpy-file:`Misc/externals.spdx.json`
SBOM file. When updating external dependencies for a CPython branch:

1. Push the update to the ``cpython-source-deps`` repository and create a new git tag.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth noting that this can only be done by a core committer, and we don't accept PRs for it (because we need to verify the sources have come from the right source and are unmodified, and our trust boundary for this is "has the commit bit").

Also might be worth noting that sometimes there's a build step involved and the core committer will then push a tag to cpython-bin-deps that will actually be used in the build. Tcl/Tk, libffi and OpenSSL are all in this group.

In practice for contributors, what this usually means is that they should post an issue requesting the updated version, wait for a core dev to say the tags are ready, and then the contributor can continue with the following steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this comment in b32b691. Do you think we should cover the cpython-bin-deps part here as well?

Copy link
Member

Choose a reason for hiding this comment

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

Not in the same note, but it ought to be documented somewhere. At the very least, we should mention the cpython-bin-deps repo at least once so that someone reading this knows to look there.

developer-workflow/sbom.rst Outdated Show resolved Hide resolved
developer-workflow/sbom.rst Outdated Show resolved Hide resolved
developer-workflow/sbom.rst Outdated Show resolved Hide resolved
developer-workflow/sbom.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Overall, this is a nice improvement. Perhaps adding subsections would add context and clarity (not suggested wording but I see 3 distinct parts):

  • Process for updating dependencies: who and how (make a subsection and not a note)
  • Background on how the SBOM is built
  • Steps for a core dev updating the external dependencies

developer-workflow/sbom.rst Show resolved Hide resolved
developer-workflow/sbom.rst Outdated Show resolved Hide resolved
developer-workflow/sbom.rst Outdated Show resolved Hide resolved
developer-workflow/sbom.rst Show resolved Hide resolved
developer-workflow/sbom.rst Outdated Show resolved Hide resolved
builds of CPython for Windows in the script :cpy-file:`PCbuild/get_externals.bat`.

In this script the libraries to fetch are designated by ``{name}-{version}``
Git refs being added to the ``libraries`` variable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be helpful to clarify where the libraries variable is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still unclear.

@sethmlarson
Copy link
Contributor Author

@willingc Apologies, didn't mean to mark the PR as ready for more review. I won't be able to get this one complete until later in March after I'm back from a trip.

@willingc
Copy link
Collaborator

Ping @sethmlarson. What do we need to do to reboot this PR or move it to draft status? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants