[libcxx-commits] [PATCH] D148753: [libcxx] convert symbol checker from CMake target to lit test

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/lib/abi/CMakeLists.txt:60
 
-  if (EXISTS "${abi_list_file}")
-    add_custom_target(check-cxx-abilist
-      "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/utils/sym_diff.py"
-          --only-stdlib-symbols
-          --strict "${abi_list_file}"
-          $<TARGET_FILE:cxx_shared>
-      DEPENDS cxx_shared
-      COMMENT "Testing libc++'s exported symbols against the ABI list")
-  else()
-    message(STATUS "ABI list file not generated for configuration ${abi_list_identifier}, `check-cxx-abilist` will not be available.")
-  endif()
-
-  add_custom_target(generate-cxx-abilist
-    COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/utils/generate_abi_list.py"
-            --output "${abi_list_file}"
-            "$<TARGET_FILE:cxx_shared>"
-    DEPENDS cxx_shared
-    COMMENT "Generating the ABI list file for configuration ${abi_list_identifier}")
-else()
-  message(STATUS "Not building a shared library for libc++ -- the ABI list targets will not be available.")
+ if (NOT EXISTS "${abi_list_file}")
+   add_custom_target(generate-cxx-abilist
----------------
francii wrote:
> ldionne wrote:
> > francii wrote:
> > > If we go this route, the call to the `check-cxx-abilist` target will need to be removed from `run-buildbot`
> > I think we should unconditionally have a `generate-cxx-abilist` target available. I'm not sure why we'd want to disable it sometimes?
> Based on my understanding, does it make sense to move the condition inside the target instead of removing it?:
> 
> Let's say after a build, you always generate the `.abilist` file (overwriting the existing one). When you run the lit test, you will be comparing the newly-built library against the `.abilist` file that was generated from the same library. Wouldn't this test always pass?
> 
> Hence why I kept the condition from the original file. This way, the lit test will compare the new library against the existing abi list file, and if the developer wants to update the abi list file they can run the script manually. Alternatively, we can update the lit test to do the generation if the ABI has changed.
We always want the custom target to exist, but we don't always want to run it. We should indeed only run it manually, but it should always be defined.

I agree that it doesn't make sense to run the `generate-cxx-abilist` target before running the test, but we're not doing that right now (nor in this patch AFAICT).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148753/new/

https://reviews.llvm.org/D148753