Make 'make check' more consistent, and better documented, across projects #566
Conversation
|
Build succeeded.
|
Makefile
Outdated
| if ! test -x /usr/bin/tox ; then sudo dnf -y install python3-tox ; fi | ||
| if ! test -x /usr/bin/requre-patch ; then sudo dnf -y install python3-requre ; fi | ||
| if ! test -d /usr/share/doc/python3-flexmock ; then sudo dnf -y install python3-flexmock ; fi |
bcrocker15
changed the title
bcrocker15
force-pushed
the
ogr-makefile-update-prepare-check
branch
from
00b73cb to
f339c39
Compare
|
Build succeeded.
|
Makefile
Outdated
| @@ -4,12 +4,13 @@ PY_PACKAGE := ogr | |||
| OGR_IMAGE := ogr | |||
|
|
|||
| build: recipe.yaml | |||
| if ! test -x /usr/bin/ansible-bender ; then sudo dnf -y install ansible-bender ; fi | |||
There was a problem hiding this comment.
I think -y should be removed here, as this might potentially install software on someones system, and they might want to first review what is installed.
Also, mind that by doing this, these make targets won't be usable on any other Linux distro then Fedora Linux and family (where dnf is used for package management). Sometimes we have contributors who are using Ubuntu or even MacOS. So having a list of dependencies in CONTRIBUTING.md would be more useful for them. In part, this was the reason I was arguing to create a separate target to install the required tooling, independent of make check.
And lastly: if dnf is used to install packages, probably using rpm to check if those packages are already installed would be semantically more correct and more resilient.
There was a problem hiding this comment.
I agree about (dnf) -y, EXCEPT that Zuul really hates being presented
with the interactive dialogue; see the results of the latest push
below, summarized here:
...
Install 2 Packages
Total download size: 229 k
Installed size: 891 k
Is this ok [y/N]: Operation aborted.
make: *** [Makefile:20: check] Error 1
At the same time, I introduced some code to detect whether the OS is
Fedora and set (new) PACKAGE_CHECKER and PACKAGE_INSTALLER
accordingly.
I totally agree about using rpm (rpm -q --quiet) to check whether
packages are already installed, so PACKAGE_CHECKER = rpm -q --quiet
on Fedora.
There was a problem hiding this comment.
I agree about (dnf) -y, EXCEPT that Zuul really hates being presented
with the interactive dialogue; see the results of the latest push
below, summarized here:
...
Install 2 PackagesTotal download size: 229 k Installed size: 891 k Is this ok [y/N]: Operation aborted. make: *** [Makefile:20: check] Error 1
Yet another argument to have a separate make target to install dependencies and one to run the tests.
There was a problem hiding this comment.
So I will reintroduce the 'prepare-check' target and move the
dependency checks back under it, and I'll use similar constructs
in our other projects.
bcrocker15
force-pushed
the
ogr-makefile-update-prepare-check
branch
from
f339c39 to
bfb3af2
Compare
|
Build failed.
|
bcrocker15
force-pushed
the
ogr-makefile-update-prepare-check
branch
from
bfb3af2 to
1960705
Compare
|
Build succeeded.
|
|
@bcrocker15 I see the tests are passing now, is this ready for review? |
bcrocker15
force-pushed
the
ogr-makefile-update-prepare-check
branch
from
1960705 to
ab13c63
Compare
|
Build failed.
|
|
regate |
CONTRIBUTING.md
Outdated
| NOTE that 'make build' uses approximately 7.1 GB of disk space; see /etc/containers/storage.conf | ||
| for the location of this storage. It is typically /var/lib/containers/storage for root, | ||
| $HOME/.local/share/containers/storage for non-root users. ('make check-in-container' does not use | ||
| appreciably more storage.) |
There was a problem hiding this comment.
I think this information could get out of date quickly, might vary from system to system, and most of it is a repetition of the podman/buildah documentation. But there is some value mentioning that building the container image will require a handful of GB of storage. Maybe mention just that?
There was a problem hiding this comment.
I acknowledge that the required disk storage, and its location, might
change over time. But I think we have to start somewhere, and I think
we need both pieces of information; as a user, my first question after
learning that OGR 'make check' requires 7.1 GB of storage space would
be "WHERE do I need 7.1 GB free?"
Please note also that the buildah manpage itself recapitulates
information from the podman manpage:
buildah:
--root value
Storage root dir (default: "/var/lib/containers/storage" for UID 0,
"$HOME/.local/share/containers/storage" for other users) Default root
dir is configured in /etc/containers/storage.conf
podman:
--root=value
Storage root dir in which data, including images, is stored (default:
"/var/lib/containers/storage" for UID 0, "$HOME/.local/share/contain‐
ers/storage" for other users). Default root dir configured in
/etc/containers/storage.conf.
There was a problem hiding this comment.
Buildah and podman are sister projects, they track each other closely and share many of the developers. We don't do either, so duplicating information from their man pages instead of referencing it, creates the risk of ultimately giving incorrect or misleading guidance for future contributors of this project. In my view it is outside of the scope of this contribution guide to teach its readers about container tooling and how to configure it, especially if that tooling has appropriate documentation.
Please update the note to the following:
NOTE that 'make build' requires a handful of GB of storage. Consult `man containers-storage.conf` to learn more about container storage paths (usually, for rootless users) and configuration.
ab13c63 to
5fd32ed
Compare
|
Build failed.
|
|
recheck should be fixed now |
|
regate |
1 similar comment
|
regate |
csomh
added
mergeit
|
recheck |
|
Build failed.
|
|
recheck |
|
Build failed.
|
Introduce IS_FEDORA to record whether we're building on Fedora, and to introduce a model for conditionalizing on OS; introduce PACKAGE_CHECKER to check for presence of package, and PACKAGE_INSTALLER to install a package. In 'prepare-check' target, check for and conditionally install: - python3-flexmock - python3-requre depending on presence/absence of /usr/share/doc/python3-flexmock and /usr/bin/requre-patch respectively. Introduce 'prepare-build' target, check for and conditionally install: - ansible-bender - podman in 'build' target. Remove all mention of unused python3-tox. CONTRIBUTING.md: Remove all mention of unused tox. Document installation of python3-flexmock and python3-requre by 'make prepare-check', ansible-bender and podman by 'make prepare-build'. Include note about multiple GB of disk space consumed by 'make build'. Mention conditional installation of - ansible-bender - podman - python3-requre - python3-flexmock Minor language & spelling fixes. Fixes OGR issue packit#579 Signed-off-by: Ben Crocker <[email protected]>
bcrocker15
force-pushed
the
ogr-makefile-update-prepare-check
branch
from
5fd32ed to
b834535
Compare
|
Build succeeded.
|
|
regate |
|
Build succeeded (gate pipeline).
|
Makefile:
Abolish the 'prepare-check' target, merging its functionality into
'check' target. In 'check' target, conditionally install
depending on presence/absence of /usr/share/doc/python3-flexmock
and /usr/bin/requre-patch respectively.
Check for
in 'build' target.
Remove all mention of unused python3-tox.
CONTRIBUTING.md:
Remove all mention of unused tox.
Remove mention of prepare-check (merged into 'check' target).
Document installation of python3-flexmock and python3-requre
by 'make check', ansible-bender and podman by 'make build'.
Include note about 7.1 GB of disk space consumed by 'make build'.
Mention conditional installation of
Minor language & spelling fixes.
Fixes #579