-
Notifications
You must be signed in to change notification settings - Fork 4
Integrate fenv into build system #242
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
Closed
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit updates the build system to use fenv (FoundationDB development environment) for consistent Docker-based builds and testing. Changes: - Add fenv as a git submodule - Create docker/Dockerfile.fenv extending fenv base image with project-specific build tools (Go, golangci-lint, shellcheck, hadolint, jq, pandoc) - Update build.sh to use fenv.sh for build/verify/generate tasks - Update CLAUDE.md documentation to reflect fenv integration Benefits: - Standardized FoundationDB development environment - Automatic FDB cluster management for integration tests - Consistent behavior between local and CI environments - Persistent cache directory for faster builds
This commit eliminates duplication by merging docker/Dockerfile and docker/Dockerfile.fenv into a single multi-stage Dockerfile. Changes: - Refactor docker/Dockerfile with three stages: 1. fenv-builder: Extends fenv base image with FQL build tools 2. gobuild: Compiles the FQL binary 3. final: Minimal runtime image with FDB client and binary - Remove docker/Dockerfile.fenv (now obsolete) - Update build.sh to use consolidated Dockerfile with --target flag - Update bake.hcl to target fenv-builder stage and add FENV_DOCKER_TAG - Update CLAUDE.md to reflect consolidated build architecture Benefits: - Single source of truth for all Docker builds - No duplication of build tool installation - Same Dockerfile used by both fenv (development) and final image (runtime) - Easier to maintain and update dependencies
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
This commit addresses all review comments from PR #242: 1. Rename 'fenv-builder' to 'builder' for consistency 2. Remove git config (fenv base image already handles this) 3. Remove jq installation (fenv handles database initialization) 4. Consolidate fdb-installer stage into final stage (simpler) 5. Remove bake.hcl (defaults already in Dockerfile) 6. Combine FENV_SCRIPT and FENV_FLAGS into FENV_CMD variable 7. Remove setup_database.sh (fenv handles this automatically) Changes: - docker/Dockerfile: Rename stage, remove jq, remove git config, consolidate FDB download into final stage - build.sh: Combine fenv variables into FENV_CMD, remove database setup call, replace bake with direct docker build - bake.hcl: Removed (no longer needed) - scripts/setup_database.sh: Removed (fenv handles this) - scripts/docker_tag.sh: Update comment (no longer references bake.hcl) - .github/workflows/verify.yml: Replace bake action with build.sh - CLAUDE.md: Update documentation to reflect changes Benefits: - Simpler build system with less duplication - Relies on fenv for database management - Direct docker build instead of bake abstraction - Fewer files to maintain
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
janderland
commented
Dec 28, 2025
This commit addresses the second round of PR feedback:
1. Remove redundant installations from Dockerfile
- fenv base image already provides: build-essential, ca-certificates,
git, curl, shellcheck, hadolint
- Only install what's unique to FQL: Go, golangci-lint, pandoc
2. Use FENV_FDB_VER everywhere instead of FDB_VER
- Replace FDB_VER with FENV_FDB_VER in build.sh
- Update GitHub workflow to use FENV_FDB_VER
- Generate FDB_LIB_URL dynamically in Dockerfile from FENV_FDB_VER
3. Remove scripts/docker_tag.sh
- Use fenv's docker_tag.sh instead
- Update build.sh to call ./fenv/docker_tag.sh
4. Simplify build.sh help message
- Remove explanatory text about fenv
Changes:
- docker/Dockerfile: Remove redundant apt-get, shellcheck, hadolint;
generate FDB URL from FENV_FDB_VER instead of taking FDB_LIB_URL param
- build.sh: Use FENV_FDB_VER, call fenv/docker_tag.sh, remove fenv
explanation from help, remove FDB_LIB_URL build arg
- scripts/docker_tag.sh: Removed (use fenv's version)
- .github/workflows/verify.yml: Replace FDB_VER with FENV_FDB_VER
Benefits:
- Simpler Dockerfile with less duplication
- Consistent use of FENV_FDB_VER throughout
- Leverage fenv's versioning infrastructure
The workflow was failing because it didn't checkout the fenv submodule, which is now required since build.sh calls: - ./fenv/docker_tag.sh for versioning - ./fenv/fenv.sh for build tasks Changes: - Add 'submodules: true' to checkout action - Remove unused fdb_lib_url from matrix (no longer needed) This fixes the workflow build errors.
janderland
commented
Dec 28, 2025
.github/workflows/verify.yml
Outdated
Comment on lines
27
to
29
|
|
||
| - name: Restore cache | ||
| uses: actions/cache@v4 |
Owner
Author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to manually restore the cache. fenv takes care of this for us as well.
The workflow was failing with "Invalid flag '--target'" because fenv.sh doesn't support the --target flag. Root cause: - build.sh was calling: fenv.sh --docker ./docker/Dockerfile --target builder - fenv.sh only supports: --build, --exec, --down, --docker FILE - No --target flag exists in fenv.sh Solution: - Create separate docker/Dockerfile.builder with just the builder stage - Update build.sh to use --docker ./docker/Dockerfile.builder - Remove the unsupported --target flag This allows fenv.sh to build the correct image (builder with all build tools) without needing target selection. Changes: - docker/Dockerfile.builder: New file with builder stage only - build.sh: Use Dockerfile.builder, remove --target flag - CLAUDE.md: Update documentation to reflect new structure This fixes the workflow build errors.
Fixed docker/Dockerfile to eliminate duplication and use the pre-built
fenv-ext-fql builder image instead of rebuilding its own builder stage.
Problem:
- docker/Dockerfile.builder defines the builder stage (for fenv dev/test)
- docker/Dockerfile also had its own builder stage (duplicate definition)
- When building final fql image, docker/Dockerfile tried to use
FROM fenv:${FENV_DOCKER_TAG} but the tag was incorrect
Solution:
- Remove the duplicate builder stage from docker/Dockerfile
- Use the pre-built fenv-ext-fql image built by --image build
- Pass FENV_EXT_DOCKER_TAG (not FENV_DOCKER_TAG) to docker build
Changes:
- docker/Dockerfile: Remove builder stage, use fenv-ext-fql directly
- build.sh: Pass FENV_EXT_DOCKER_TAG=${DOCKER_TAG} to docker build
- CLAUDE.md: Update documentation to clarify the image relationship
Benefits:
- No duplication between Dockerfile.builder and Dockerfile
- Simpler build process - builder is built once via fenv
- Correct image tags are used throughout
As noted in PR feedback, fenv handles cache management internally via the CACHE_VOLUME environment variable. The manual "Restore cache" step for ~/.cache/fql is redundant because: 1. fenv.sh mounts CACHE_VOLUME (~/cache/fql) to /cache in the container 2. This directory persists on the GitHub Actions runner between steps 3. Go build artifacts and golangci-lint cache are stored there 4. No explicit cache restore/save action is needed The FDB image caching is kept as it serves a different purpose: caching the Docker image itself to avoid re-pulling from Docker Hub. Changes: - Remove "Restore cache" step from workflow - Keep CACHE_VOLUME env var for verify step - Keep FDB image caching steps (different optimization)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit updates the build system to use fenv (FoundationDB development environment) for consistent Docker-based builds and testing.
Changes:
Benefits: