Skip to content

Conversation

cgwalters
Copy link
Member

No description provided.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the build process to be more incremental, primarily by updating ci/libbuild.sh to conditionally run autogen.sh and configure. Changes are also made to the Dockerfile to use this new script, and a build-host target is added to the Justfile.

The review highlights a critical concern regarding the apparent removal of ccache utilization, which could impact build performance. Additionally, a minor file formatting issue in the Justfile and a point of clarity regarding a verbosity parameter in the Dockerfile are noted. The core logic for incremental builds appears sound.

env ./configure \
--sysconfdir=/etc --prefix=/usr --libdir=/usr/lib64 \
--with-openssl --with-selinux --with-composefs \
. ci/libbuild.sh

Choose a reason for hiding this comment

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

high

The previous build setup explicitly configured and used ccache to speed up compilations. With the current changes, ccache is still installed by ci/installdeps.sh, but the environment variables CC and CCACHE_DIR are no longer set in this RUN step. This means ccache will likely not be utilized, potentially slowing down builds.

To reinstate ccache functionality, you can set CC and CCACHE_DIR before invoking the build script. The existing cache mount --mount=type=cache,target=/build/target can be leveraged to store ccache data, for example, in /build/target/.ccache.

export CCACHE_DIR="/build/target/.ccache" # Store ccache data within the mounted cache
mkdir -p "${CCACHE_DIR}"
export CC="ccache gcc" # Or your specific compiler, e.g., ccache clang
. ci/libbuild.sh

Dockerfile Outdated
--sysconfdir=/etc --prefix=/usr --libdir=/usr/lib64 \
--with-openssl --with-selinux --with-composefs \
. ci/libbuild.sh
build V=0 --with-openssl --with-selinux --with-composefs \

Choose a reason for hiding this comment

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

medium

The V=0 argument is passed to the build function. This argument will be included in "$@" within ci/libbuild.sh and thus passed to the ../../configure command. Standard configure scripts typically do not use V=<level> for verbosity control (makefiles do).

Meanwhile, the make command within the build function (in ci/libbuild.sh) is hardcoded as make V=1, meaning it will always run verbosely.

If the V=0 here was intended to make the make process non-verbose, this will not have the desired effect. If it was intended for configure, it's likely a no-op. Please clarify the intent of V=0 or consider adjusting how verbosity is handled if make's verbosity needs to be configurable via this parameter.

Justfile Outdated

# Run a build on the host system
build-host:
. ci/libbuild.sh && build

Choose a reason for hiding this comment

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

medium

The newly added line does not end with a newline character. It's a common convention and good practice for text files to end with a newline character. Some tools and diff utilities might behave unexpectedly or show warnings for files not ending with a newline.

    . ci/libbuild.sh && build

Since it's really convenient sometimes.

Signed-off-by: Colin Walters <[email protected]>
Avoid rerunning autoconf if we already have it cached.

Signed-off-by: Colin Walters <[email protected]>
For faster iteration.

Signed-off-by: Colin Walters <[email protected]>
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

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.

2 participants