Skip to content

fix(build): remove duplicate test targets from tests/CMakeLists.txt - #93

Open
prakhar7017 wants to merge 1 commit into
embeddedos-org:masterfrom
prakhar7017:fix/cmake-duplicate-test-targets
Open

fix(build): remove duplicate test targets from tests/CMakeLists.txt#93
prakhar7017 wants to merge 1 commit into
embeddedos-org:masterfrom
prakhar7017:fix/cmake-duplicate-test-targets

Conversation

@prakhar7017

Copy link
Copy Markdown

Problem

tests/CMakeLists.txt declared test_crypto_aes and test_crypto_sha512 twice
each — once in a group block at lines 59–74, then again individually at lines
76–84. A repeated add_executable target name is a hard CMake error, not a
warning:

CMake Error at tests/CMakeLists.txt:77 (add_executable):
add_executable cannot create target "test_crypto_aes" because another
target with the same name already exists. The existing target is an
executable created in source directory "D:/eos/tests".

CMake Error at tests/CMakeLists.txt:79 (add_test):
add_test given test NAME "test_crypto_aes" which already exists in this
directory.

CMake Error at tests/CMakeLists.txt:82 (add_executable):
add_executable cannot create target "test_crypto_sha512" because another
target with the same name already exists.

-- Configuring incomplete, errors occurred!

Configuration aborted, so the exact command CLAUDE.md and README.md give for
building the suite —

cmake -B build/host -DCMAKE_BUILD_TYPE=Debug -DEOS_BUILD_TESTS=ON

— produced no build system at all. Not one test target could be generated,
built or run. This affects every developer following the documented setup.

Changes

- tests/CMakeLists.txt — removed the duplicate add_executable /
  target_link_libraries / add_test triples for test_crypto_aes and
  test_crypto_sha512. Kept the copy whose comment describes what the suite
  actually covers (AES-128/192/256 ECB + CBC, NIST FIPS-197 / SP 800-38A,
  SHA-512 against NIST vectors), and gave test_crypto_ecc and
  test_crypto_rsa the same one-line description so the block is consistent.
- CHANGELOG.md — entry under [Unreleased] / Fixed.

No target was added or removed, and no source file or link dependency changed.
The 28 registered test targets are exactly the same set as before, each now
declared once.

Testing

No new tests. This is a build-system repair — the evidence is that configuration
now completes.

┌───────────────────────────────────────────────────────────────────────┬──────────────────────────────────────────┐
│                                 Check                                 │                  Result                  │
├───────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────┤
│ cmake -B build/pr1 -G Ninja -DCMAKE_BUILD_TYPE=Debug                  │ PASS — Configuring done / Generating     │
│ -DEOS_BUILD_TESTS=ON on this branch                                   │ done, no errors                          │
├───────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────┤
│ Same command on master                                                │ FAIL — 4 CMake errors, Configuring       │
│                                                                       │ incomplete                               │
└───────────────────────────────────────────────────────────────────────┴──────────────────────────────────────────┘

Verified with CMake 4.4.3, Ninja 1.13.0, GCC 6.3.0 (MinGW) on Windows.

tests/CMakeLists.txt declared test_crypto_aes and test_crypto_sha512
twice each. A repeated add_executable target name is a hard CMake error,
not a warning:

    CMake Error at tests/CMakeLists.txt:77 (add_executable):
      add_executable cannot create target "test_crypto_aes" because
      another target with the same name already exists.
    CMake Error at tests/CMakeLists.txt:79 (add_test):
      add_test given test NAME "test_crypto_aes" which already exists.

Configuration aborted, so the command the README and CLAUDE.md give for
building the test suite produced no build system at all and no test
could be built or run.

Kept the second copy of each pair, whose comments describe what the
suite actually covers, and gave test_crypto_ecc and test_crypto_rsa the
same one-line description for consistency. No target, source file or
link dependency changes.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The defect is real and I reproduced it on master:

$ git show origin/master:tests/CMakeLists.txt | grep -oE 'add_executable\([a-z_0-9]+' | sort | uniq -c | awk '$1>1'
      2 test_crypto_aes
      2 test_crypto_sha512

Two declarations each, and a duplicate add_executable name is a hard CMake
error, so -DEOS_BUILD_TESTS=ON cannot generate a single test target. The
CHANGELOG entry describes it accurately.

One thing you should know before this merges: #82 fixes the same two
duplicates
, as part of repairing four breakages that landed together. It takes
tests/CMakeLists.txt from 30 add_executable lines to 28; so does this. The
two will conflict.

That does not make this PR wasted, because you made the better call on which
copy survives. #82 keeps the grouped comment:

# --- test_crypto_aes / _ecc / _rsa / _sha512: were sitting in tests/ unused ---

which describes how the tests got there rather than what they cover. This keeps
the per-test descriptions:

# --- test_crypto_aes: AES-128/192/256 ECB + CBC, NIST FIPS-197 / SP 800-38A ---
# --- test_crypto_ecc: ECC point arithmetic and ECDSA ---
# --- test_crypto_rsa: RSA modular exponentiation and verify ---
# --- test_crypto_sha512: SHA-512 against NIST vectors ---

Naming the standard each vector set comes from is worth more to the next reader
than a note about the file's history. I am folding your version into #82 with
attribution, so the outcome is yours even though the conflict resolves in that
PR's favour.

Approving on the merits. If #82 lands first this becomes empty and can close; if
this lands first, #82 rebases cleanly around it.

Worth flagging that this is not the only duplicate-target problem in the
platform. eos and eBoot both define test_crypto and test_multicore, and
since ebuild composes them with add_subdirectory, the integration build could
not configure either — same class of error, one repository apart. That one is
embeddedos-org/ebuild#85, fixed in embeddedos-org/eBoot#71.

srpatcha added a commit that referenced this pull request Aug 30, 2026
Deduplicating test_crypto_aes and test_crypto_sha512 left the grouped
comment that had been sitting above the block:

    # --- test_crypto_aes / _ecc / _rsa / _sha512: were sitting in tests/ unused ---

That records how the files got here, which stops being interesting the
moment they are wired up. #93 fixes the same two duplicates and keeps the
per-test descriptions instead:

    # --- test_crypto_aes: AES-128/192/256 ECB + CBC, NIST FIPS-197 / SP 800-38A ---
    # --- test_crypto_ecc: ECC point arithmetic and ECDSA ---
    # --- test_crypto_rsa: RSA modular exponentiation and verify ---
    # --- test_crypto_sha512: SHA-512 against NIST vectors ---

Naming the standard each vector set comes from is worth more to the next
reader than a note about the file's history. Adopting that here so the
better wording survives the conflict between the two PRs.

Comment-only; no target, source or link line changes.

    28/28 tests passed, 0 build errors

Co-authored-by: prakhar7017 <prakhar7017@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants