Skip to content

Commit

Permalink
CMake: remove -DSERIALIZE_CHIPDBS= option.
Browse files Browse the repository at this point in the history
The impetus for this commit is the fact that it causes rare but
build-breaking race conditions when used with `make -jN` with `N > 1`.
These race conditions are difficult to track down or fix because of
the very rudmentary debugging tools provided by `make` and opaque
semantics of CMake's Makefile generator. They break the build by
running two `.bba` generation processes, then one of them renaming
the `.bba.new` file once it's done, leaving the other one to fail.

After reflection (as the author of this code path) and discussion with
community members who use it, I've concluded that this isn't the right
approach.
1. In practice, on targets where `-DSERIALIZE_CHIPDBS=` matters, you
   also care about other build steps, like linking nextpnr, which
   are not serializable this way. So you use a workaround anyway, like
   `make`ing individual targets instead.
2. The way to serialize the build with Make is the `-j1` option. Trying
   to work around `-jN` to make it work like `-j1` is inherently error
   prone. While there is some utility in not serializing C++ compilation
   this utility could be more easily achieved by providing a single
   target that builds all chipdbs, running `make <chipdb-target> -j1`,
   then running `make -jN` for the rest of the build.
  • Loading branch information
whitequark committed Jan 21, 2025
1 parent d755d3e commit 9caffca
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 14 deletions.
3 changes: 0 additions & 3 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ option(BUILD_TESTS "Build tests" OFF)
option(USE_OPENMP "Use OpenMP to accelerate analytic placer" OFF)
option(STATIC_BUILD "Create static build" OFF)
option(EXTERNAL_CHIPDB "Create build with pre-built chipdb binaries" OFF)
option(SERIALIZE_CHIPDBS "Serialize device data preprocessing to minimize memory use" ON)
option(WERROR "pass -Werror to compiler (used for CI)" OFF)
option(PROFILER "Link against libprofiler" OFF)
option(USE_IPO "Compile nextpnr with IPO" ON)
Expand Down Expand Up @@ -67,8 +66,6 @@ else()
set(BBASM_MODE "string")
endif()

set(BBASM_SERIALIZE ${SERIALIZE_CHIPDBS})

find_package(Threads)
if (Threads_FOUND)
if (NOT STATIC_BUILD)
Expand Down
11 changes: 0 additions & 11 deletions cmake/BBAsm.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,9 @@ function(add_bba_produce_command)
DEPENDS
${arg_EXECUTABLE}
${arg_INPUTS}
$ENV{SERIALIZE_BBA_TARGET}
VERBATIM
)

if (BBASM_SERIALIZE)
# Have to insert a custom target in between two custom commands, else CMake will try to
# depend on the previous (in serialization order) command directly, which will fail if
# they're in different directories. Unfortunately this makes the terminal output uglier.
math(EXPR next_count "$ENV{SERIALIZE_BBA_COUNT} + 1")
add_custom_target(--bbasm-serialize-${next_count} DEPENDS ${arg_OUTPUT})
set(ENV{SERIALIZE_BBA_COUNT} ${next_count})
set(ENV{SERIALIZE_BBA_TARGET} --bbasm-serialize-${next_count})
endif()

endfunction()

# Example usage:
Expand Down

0 comments on commit 9caffca

Please sign in to comment.