fix(cmake): make dash_static find libedit headers on Linux#8
Open
lcolok wants to merge 1 commit intoxicilion:masterfrom
Open
fix(cmake): make dash_static find libedit headers on Linux#8lcolok wants to merge 1 commit intoxicilion:masterfrom
lcolok wants to merge 1 commit intoxicilion:masterfrom
Conversation
dash_static's target_include_directories references LIBEDIT_INSTALL_DIR
before the variable is defined further down in the file, so the include
path effectively becomes `/include` and compilation of any dash source
that includes `myhistedit.h` (which transitively `#include <histedit.h>`)
fails on Linux with:
fatal error: histedit.h: No such file or directory
Linux is affected because the dash build requires `histedit.h` at compile
time. On macOS the same translation units still resolve the header
through other search paths, which is why this regression was not caught
on that platform.
The fix has three minimal parts:
1. Move `set(LIBEDIT_INSTALL_DIR ...)` above `add_library(dash_static)`
so the variable is defined before it is referenced.
2. Add `${LIBEDIT_INSTALL_DIR}/include` to dash_static's include dirs.
3. Add `add_dependencies(dash_static libedit_vendored)` so the libedit
external project is built (and its headers installed) before
dash_static is compiled.
Verified by:
- Clean Release build on Ubuntu 24.04 (x86_64) now succeeds and
produces a working `build/boxsh`.
- Reverting just the three added lines reliably reproduces the
`histedit.h: No such file or directory` failure during dash_static
compilation.
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
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.
Summary
On Linux, a clean
cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake --build buildagainstmaster(72bc8c2, v2.1.0) fails to compiledash_static:Root cause
CMakeLists.txtwiresdash_static's include dirs at line 73:But
LIBEDIT_INSTALL_DIR(where libedit'shistedit.hlands afterlibedit_vendoredbuilds) is only defined at line 116, sodash_statichas no visibility into libedit's headers. On top of that,dash_statichas no explicit dependency onlibedit_vendored, so CMake is free to start compiling dash sources before libedit's headers are installed.Linux reproduces this because the dash build unconditionally requires
histedit.h. macOS happens to resolve the header through another search path, which is why the regression was not caught there.Fix
Three small, additive changes to
CMakeLists.txt:set(LIBEDIT_INSTALL_DIR ${CMAKE_BINARY_DIR}/libedit-prefix)beforeadd_library(dash_static), so the variable is defined before it is used.${LIBEDIT_INSTALL_DIR}/includetotarget_include_directories(dash_static)so#include <histedit.h>resolves.add_dependencies(dash_static libedit_vendored)so the external libedit build (whichinstallshistedit.hinto that include dir) completes beforedash_staticcompilation starts.Diff is 4 lines, no logic changed elsewhere.
Verification
Tested on Ubuntu 24.04 LTS (x86_64):
With the patch — clean Release build succeeds:
Basic smoke tests all pass:
boxsh --versionboxsh version 2.1.0boxsh -c 'echo hi'hiboxsh --sandbox -c 'cat /proc/1/comm'boxsh(PID namespace active)boxsh --sandbox --new-net-ns -c 'ping -c1 -W1 8.8.8.8'Network is unreachable--bind cow:SRC:DSTwrites isolated from SRCReverse verification — stashing the patch reliably reproduces the failure:
Risk
Low. The change only reorders a CMake variable definition and adds one include path plus one
add_dependenciesedge for a target that already depends onlibeditat link time viaboxsh. No runtime behavior changes.