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

docs: include required tools in source tree #4228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented Apr 26, 2023

In order to be able to build the documentation without internet access
(as is required by some distribution build systems), all of the source
code needed for the build needs to be available in the source tarball.

This used to be possible with the docker-cli sources but was
accidentally broken with some CI changes that switched to downloading
the tools (by modifying go.mod as part of the docs build script).

This pattern also maked documentation builds less reproducible since the
tool version used was not based on the source code version.

Fixes: commit 7dc35c0 ("validate manpages target")
Fixes: commit a650f4d ("switch to cli-docs-tool for yaml docs generation")
Fixes #4212
See #3381
Signed-off-by: Aleksa Sarai [email protected]

@cyphar cyphar requested a review from thaJeztah as a code owner April 26, 2023 00:26
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2023

Codecov Report

Merging #4228 (8324502) into master (cd6467b) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4228   +/-   ##
=======================================
  Coverage   59.70%   59.70%           
=======================================
  Files         288      288           
  Lines       24817    24817           
=======================================
  Hits        14818    14818           
  Misses       9113     9113           
  Partials      886      886           

@cyphar cyphar force-pushed the docs-no-internet branch 3 times, most recently from e926e70 to bf5f804 Compare April 26, 2023 00:46
@cyphar
Copy link
Contributor Author

cyphar commented Apr 26, 2023

I'm confused why the doc building validation is failing with Go compilation errors:

go: cannot find main module, but -modfile was set.
	-modfile cannot be used to set the module root directory.

On my machine, with this patch both make manpages and make -f docker.Makefile manpages work without issues. Same goes for yamldocs and mddocs. I'm also using go1.20.3.

@thaJeztah
Copy link
Member

It needs a temporary go.mod, otherwise -modfile=vendor.mod won't work.

But we need to look at this change; I know we split these to separate modules to not add the tool's dependencies as dependencies for the main module / vendor, and because we needed these separate modules for our migration to Hugo for the docs website. ./cc @crazy-max @dvdksn

@dvdksn
Copy link
Contributor

dvdksn commented Apr 26, 2023

I don't think my change had any impact in this respect - it was only moving the doc generation to a subdirectory so it wouldn't confuse Hugo when fetching content files from this repo.

The cli-docs-tool module needs to be an external dep since it's used by other projects as well. I don't suppose there's much harm in vendoring them here -- other than 23k lines of bloat.

@cyphar
Copy link
Contributor Author

cyphar commented Apr 26, 2023

It needs a temporary go.mod, otherwise -modfile=vendor.mod won't work.

Ah. I can add the temporary copying logic for these builds if it's necessary.

I know we split these to separate modules to not add the tool's dependencies as dependencies for the main module / vendor

Unfortunately that's exactly what I'm trying to revert -- removing this from vendoring means that we cannot build Docker's man pages in openSUSE's OBS (there is no internet access in OBS when building packages).

The other alternative would be to include the generated of the man pages in the source tree (verifying they are kept up-to-date in the CI), so distributions can just copy them without needing to use a tool to build them during packaging.

@cyphar
Copy link
Contributor Author

cyphar commented May 29, 2023

@thaJeztah I've fixed the go.mod issue, it all works now. 😸

@cyphar
Copy link
Contributor Author

cyphar commented Sep 14, 2023

Rebased.

@thaJeztah Did you want me to go the route of making the documentation be pre-built and then verified in CI? I think packaging things this way is better for distributions (it Just Works ™️ with the distributed tarballs) and the bloat here is only to the tarballs not the final binary, but if it is an issue I can go the other route. Rebasing this patchset for every Docker patch release is getting a little tiring. 😅

In order to be able to build the documentation without internet access
(as is required by some distribution build systems), all of the source
code needed for the build needs to be available in the source tarball.

This used to be possible with the docker-cli sources but was
accidentally broken with some CI changes that switched to downloading
the tools (by modifying go.mod as part of the docs build script).

This pattern also maked documentation builds less reproducible since the
tool version used was not based on the source code version.

Fixes: commit 7dc35c0 ("validate manpages target")
Fixes: commit a650f4d ("switch to cli-docs-tool for yaml docs generation")
Signed-off-by: Aleksa Sarai <[email protected]>
@tianon
Copy link
Contributor

tianon commented May 23, 2024

Gentle ping @thaJeztah @crazy-max 🙇

(having the script that generates the manpages also be responsible for downloading and building the tools necessary is definitely problematic 😭)

@tianon
Copy link
Contributor

tianon commented May 23, 2024

If we want to avoid having the tools in the "main" package dependencies/vendoring, perhaps we could have a separate tools directory with a vendor.mod that includes these as a separate vendoring tree? Or within that "manpage generation" code itself (further isolating it in the directory tree if necessary)?

@thaJeztah
Copy link
Member

Ah, yes, I tried doing a rebase, and some updates in #4903 some time ago, but now that the cli-docs-tool also should be able to do the man-page generating (without building an go-md2man binary), we may be able to get rid of that part; but I didn't get round to that yet 😓

I think @crazy-max is on PTO this week, but maybe @krissetto or @Benehiko (also on PTO this week 🙈) are interested to have a peek.

@cyphar
Copy link
Contributor Author

cyphar commented May 24, 2024

I've been rebasing this for the SUSE packages, I can push a new version here if you like.

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

Successfully merging this pull request may close these issues.

Documentation generation without internet connectivity
5 participants