Skip to content

Conversation

makslevental
Copy link
Contributor

@makslevental makslevental commented Sep 10, 2025

This PR adds a test that exercises standalone build; test install, which tests building the standalone example using an installed distribution.

@makslevental makslevental force-pushed the users/makslevental/standalone-install branch 12 times, most recently from 6cff170 to 6b77807 Compare September 11, 2025 05:34
Copy link

github-actions bot commented Sep 11, 2025

✅ With the latest revision this PR passed the Python code formatter.

@makslevental makslevental force-pushed the users/makslevental/standalone-install branch 4 times, most recently from 56936a0 to efe7ace Compare September 11, 2025 06:05
@makslevental makslevental force-pushed the users/makslevental/standalone-install branch from 857cb13 to dcc9b4a Compare September 11, 2025 21:03
@makslevental makslevental changed the base branch from main to users/makslevental/mono September 11, 2025 21:03
@makslevental makslevental marked this pull request as ready for review September 11, 2025 21:29
@makslevental makslevental force-pushed the users/makslevental/standalone-install branch 23 times, most recently from 78a1f90 to d6011b6 Compare September 16, 2025 06:47
-D CMAKE_EXE_LINKER_FLAGS="-no-pie" \
-D LLVM_ENABLE_WERROR=ON
-D LLVM_ENABLE_WERROR=ON \
-DLLVM_INSTALL_UTILS=ON
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still necessary with your new approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true it's not but it actually revealed (indirectly) a failure mode - because the monolithic script is touched, all the projects are added, and the "layering issues" are revealed.

get_property(MLIR_EXPORTS GLOBAL PROPERTY MLIR_EXPORTS)
if(CLANG_ENABLE_CIR)
list(APPEND MLIR_EXPORTS cir-opt cir-translate cir-lsp-server)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are these? Having references to cir in MLIR looks like some layering issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a layering issue in the whole project (or possibly just CIR) then because this is the current error:

DLLVM_EXTERNAL_LIT="/home/gha/actions-runner/_work/llvm-project/llvm-project/build/bin/llvm-lit"  -DMLIR_DIR="/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir"  -DLLVM_DIR="/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/llvm"  -DLLVM_USE_LINKER=lld  -DPython3_EXECUTABLE="/usr/bin/python3"  -DPython_EXECUTABLE="/usr/bin/python3" | tee -a /home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/test/Examples/standalone/Output/test.install-dir.toy.tmp
2025-09-16T06:56:36.4951832Z # executed command: /usr/bin/cmake /home/gha/actions-runner/_work/llvm-project/llvm-project/mlir/examples/standalone -G Ninja -DCMAKE_CXX_COMPILER=/opt/llvm/bin/clang++ -DCMAKE_C_COMPILER=/opt/llvm/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DLLVM_EXTERNAL_LIT=/home/gha/actions-runner/_work/llvm-project/llvm-project/build/bin/llvm-lit -DMLIR_DIR=/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir -DLLVM_DIR=/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/llvm -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE=/usr/bin/python3 -DPython_EXECUTABLE=/usr/bin/python3
2025-09-16T06:56:36.4956077Z # .---command stderr------------
2025-09-16T06:56:36.4956844Z # | CMake Error at /usr/share/cmake-3.28/Modules/CMakeDetermineSystem.cmake:246 (configure_file):
2025-09-16T06:56:36.4957771Z # |   No such file or directory
2025-09-16T06:56:36.4958268Z # | Call Stack (most recent call first):
2025-09-16T06:56:36.4958780Z # |   CMakeLists.txt:2 (project)
2025-09-16T06:56:36.4959174Z # | 
2025-09-16T06:56:36.4959483Z # | 
2025-09-16T06:56:36.4960635Z # | CMake Error at /home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir/MLIRTargets.cmake:4432 (message):
2025-09-16T06:56:36.4962030Z # |   The imported target "cir-lsp-server" references the file
2025-09-16T06:56:36.4962659Z # | 
2025-09-16T06:56:36.4963607Z # |      "/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/bin/cir-lsp-server"
2025-09-16T06:56:36.4964887Z # | 
2025-09-16T06:56:36.4965299Z # |   but this file does not exist.  Possible reasons include:
2025-09-16T06:56:36.4965753Z # | 
2025-09-16T06:56:36.4966128Z # |   * The file was deleted, renamed, or moved to another location.
2025-09-16T06:56:36.4966606Z # | 
2025-09-16T06:56:36.4967001Z # |   * An install or uninstall procedure did not complete successfully.
2025-09-16T06:56:36.4967499Z # | 
2025-09-16T06:56:36.4967832Z # |   * The installation package was faulty and contained
2025-09-16T06:56:36.4968249Z # | 
2025-09-16T06:56:36.4969099Z # |      "/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir/MLIRTargets.cmake"
2025-09-16T06:56:36.4970134Z # | 
2025-09-16T06:56:36.4970407Z # |   but not all the files it references.
2025-09-16T06:56:36.4971024Z # | 
2025-09-16T06:56:36.4971292Z # | Call Stack (most recent call first):
2025-09-16T06:56:36.4972427Z # |   /home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/mlir/mlir-standalone-test-install/lib/cmake/mlir/MLIRConfig.cmake:37 (include)
2025-09-16T06:56:36.4973903Z # |   CMakeLists.txt:9 (find_package)
2025-09-16T06:56:36.4974252Z # | 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I.e., when LLVM_ENABLE_PROJECTS is all projects, then MLIRTargets includes CIR targets becaus CIR uses add_mlir_tool:

add_mlir_tool(cir-lsp-server

Copy link
Collaborator

@joker-eph joker-eph Sep 16, 2025

Choose a reason for hiding this comment

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

Is the issue a mismatch between MLIRTargets and the global property MLIR_EXPORTS here? Should they just match? (and so the fix is rather in ensuring that they match rather than patching it back here)

Or maybe cir should not use add_mlir_tool...

Copy link
Collaborator

@joker-eph joker-eph Sep 16, 2025

Choose a reason for hiding this comment

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

cir-lsp-server is the only thing outside of mlir/ using add_mlir_tool, cir-opt is using add_clang_tool, can we just update cir-lsp-server ?

(another reason for cir-opt / cir-translate to be in this list?)

Copy link
Member

Choose a reason for hiding this comment

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

+1 to just updating and removing CIR related code here.

)
set_target_properties(check-mlir-build-only PROPERTIES FOLDER "MLIR/Tests")

get_property(LLVM_EXPORTS GLOBAL PROPERTY LLVM_EXPORTS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused?

if(CLANG_ENABLE_CIR)
list(APPEND MLIR_EXPORTS cir-opt cir-translate cir-lsp-server)
endif()
add_custom_target(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be very clearly document here (the "why" and the "how")

COMMAND ${CMAKE_COMMAND} -E make_directory "${MLIR_STANDALONE_INSTALL_TEST_PREFIX}"
COMMAND "${CMAKE_COMMAND}"
-DCMAKE_INSTALL_PREFIX="${MLIR_STANDALONE_INSTALL_TEST_PREFIX}"
-P "${MLIR_BINARY_DIR}/cmake_install.cmake"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would think this command to be already pretty encompassing in itself, why are the other ones all needed below? (mlir-headers for example?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you check the failing tests you will see the the union of COMMANDs here is not sufficient. I am currently trying to figure out if this can actually work at all.

-DCMAKE_INSTALL_PREFIX="${MLIR_STANDALONE_INSTALL_TEST_PREFIX}"
-P "${CMAKE_BINARY_DIR}/cmake_install.cmake"
)
list(APPEND MLIR_TEST_DEPENDS install-mlir-standalone-test-prefix)
Copy link
Collaborator

@joker-eph joker-eph Sep 16, 2025

Choose a reason for hiding this comment

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

I suspect we only need this if testing the examples is enabled (that is LLVM_BUILD_EXAMPLES is the guard I believe)

Copy link
Member

@jpienaar jpienaar left a comment

Choose a reason for hiding this comment

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

Looks reasonable, but I'd echo I'm not opposed to just having this be on a buildbot (e.g., little bot that just does an install and runs a test using a shell script, doesn't need FileCheck or anything really) as I don't think the rollback was terribly bad (and rollbacks happen), and this could easily have been addressed earlier with a buildbot. E.g., I think the thing that resulted in rollback being slow was folks thinking "well is this due to my setup being wonky, what are the expectations here". While if we had a buildbot that failed, it would have been obvious, and we'd have rolled back faster. This being fast now is good and makes it viable for presubmit.

get_property(MLIR_EXPORTS GLOBAL PROPERTY MLIR_EXPORTS)
if(CLANG_ENABLE_CIR)
list(APPEND MLIR_EXPORTS cir-opt cir-translate cir-lsp-server)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

+1 to just updating and removing CIR related code here.

@makslevental makslevental force-pushed the users/makslevental/standalone-install branch from d6011b6 to bd9aabf Compare September 17, 2025 18:11
@makslevental makslevental force-pushed the users/makslevental/standalone-install branch from bd9aabf to 8f0f30d Compare September 17, 2025 18:11
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.

7 participants