Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kokkos: Remove TriBITS Kokkos subpackages (trilinos/Trilinos#11545) #6104

Merged

Conversation

bartlettroscoe
Copy link
Contributor

These are the same commits for Kokkos as part of Trilinos PR:

Therefore, this PR must be merged before Kokkos is snapshotted into Trilinos again after the merge of trilinos/Trilinos#11808 to Trilinos 'develop'.

* Removed the listing of subpackages from kokkos/cmake/Dependencies.cmake

* Remove the now-unused files
  kokkos/[core,containers,algorithms,simd]/cmake/Dependencies.cmake

* Removed TriBITS macros for a package with subpackages and replace with those
  for a package with no subpackages.  Also, removed all subpackage macros.

* Changed kokkos_process_subpackage() to just call add_subdirectory().

* Added prefix 'Core' to several tests in
  kokkos/Core/unit_tests/CMakeLists.txt now that prefix is 'Kokkos_'

* Added prefix 'Containers' to several tests in
  kokkos/containers/unit_tests/CMakeLists.txt and
  kokkos/containers/performance_tests/CMakeLists.txt now that prefix is
  'Kokkos_'

* Change name of the kokkos/containers/performance_tests/CMakeLists.txt file
  test 'PerformanceTest_XXX' to 'ContainersPerformanceTest_XXX'.

* Added prefix 'Algorithms' to several tests in
  kokkos/algorithms/unit_tests/CMakeLists.txt now that prefix is 'Kokkos_'

* Removed the usage of tribits_configure_file() and wrapper
  kokkos_configure_file() and just call configure_file().  The location of
  PACKAGE_SORUCE_DIR changed so the calls to tribits_configure_file() no
  longer worked.  (Also, these X_config.h.in files were not using any of the
  TriBITS-supported features that needed the calling of
  tribits_configure_file() so there was no reason to not just call raw
  configure_file().)

SQUASH AGINST: Kokkos: Remove TriBITS subpackages (#11545)
…nos#11545)

This restores the building of the raw CMake build of Kokkos after the
refactoring to remove TriBITS subpackages.
@dalg24-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@dalg24 dalg24 changed the title Kokkos: Remove TriBITS subpackages (#11545) Kokkos: Remove TriBITS subpackages (trilinos#11545) May 4, 2023
@bartlettroscoe bartlettroscoe changed the title Kokkos: Remove TriBITS subpackages (trilinos#11545) Kokkos: Remove TriBITS Kokkos subpackages (trilinos/Trilinos#11545) May 4, 2023
…os/Trilinos#11545)

This gives a full passing build and tests with the Trilinos PR GenConfig
clang-11.0.1 build configuration.
@bartlettroscoe
Copy link
Contributor Author

Hum, some of these GHA jobs don't seem to be testing the correct version of the code. For example, looking at the job:

it shows the commit being tested as the merge commit 0690c1b here:

The job reports the failure:

-- Configuring done (6.7s)
CMake Error at core/unit_test/CMakeLists.txt:1129 (file):
  Error evaluating generator expression:

    $<TARGET_FILE_DIR:KokkosCore_UnitTest_DeviceAndThreads>

  No target "KokkosCore_UnitTest_DeviceAndThreads"


-- Generating done (0.3s)
CMake Generate step failed.  Build files cannot be regenerated correctly.
Error: Process completed with exit code 1.

at:

But if you look at that line of code core/unit_test/CMakeLists.txt:1129 for that commit it shows:

file(GENERATE
OUTPUT $<TARGET_FILE_DIR:Kokkos_CoreUnitTest_DeviceAndThreads>/TestDeviceAndThreads.py
INPUT TestDeviceAndThreads.py
${USE_SOURCE_PERMISSIONS_WHEN_SUPPORTED}
)

Where the heck is KokkosCore_UnitTest_DeviceAndThreads coming from?

Can someone please help to explain this?

@dalg24
Copy link
Member

dalg24 commented May 5, 2023

We test the merge commit when there is no conflict but this is a red herring. I pulled your branch and it yields that error which I also don't understand at this time.

@dalg24
Copy link
Member

dalg24 commented May 5, 2023

I fixed the issue.

This PR renames our targets and executable names

KokkosCore_UnitTest_Cuda                -> Kokkos_CoreUnitTest_Cuda
KokkosAlgorithms_UnitTest_RandomAndSort -> Kokkos_AlgorithmsUnitTest_RandomAndSort
etc.

Note that in the current version KokkosSimd_UnitTest_SIMD -> Kokkos_UnitTest_SIMD (Simd package name is dropped).

One thing that we need to discuss is that this currently implies a departure from the exe names produced by the makefiles.

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

I am ok with the name change. I also don't see a pressing reason to change the Makefile System test names. All in all this gets rid of a bunch of cmake so that sounds good to me.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Apart from the question of renaming tests, this looks good to me.

Comment on lines +21 to +22
PREFIX = "$<TARGET_FILE_DIR:Kokkos_CoreUnitTest_DeviceAndThreads>"
EXECUTABLE = "$<TARGET_FILE_NAME:Kokkos_CoreUnitTest_DeviceAndThreads>"
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 does not seem like this change is important to bring back to the Trilinos copy of Kokkos. I can can use 'git format-patch' and 'git am' to bring it back just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dalg24
Copy link
Member

dalg24 commented May 5, 2023

OK to test

Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Fine with it but want to wait for CI

@dalg24
Copy link
Member

dalg24 commented May 5, 2023

ctest reports 100% of the tests passed in the SYCL build but it somehow reports failure. I will ignore.

@dalg24 dalg24 merged commit 5fa72b5 into kokkos:develop May 5, 2023
27 of 28 checks passed
@ndellingwood ndellingwood mentioned this pull request May 9, 2023
@ndellingwood
Copy link
Contributor

I added an entry to the CHANGELOG 4.1.0 on #5902 listed under

Build System Changes
Incompatibilities (i.e. breaking changes)

nliber pushed a commit to nliber/kokkos that referenced this pull request Jun 22, 2023
…okkos#6104)

* Kokkos: Remove TriBITS subpackages (#11545)

* Removed the listing of subpackages from kokkos/cmake/Dependencies.cmake

* Remove the now-unused files
  kokkos/[core,containers,algorithms,simd]/cmake/Dependencies.cmake

* Removed TriBITS macros for a package with subpackages and replace with those
  for a package with no subpackages.  Also, removed all subpackage macros.

* Changed kokkos_process_subpackage() to just call add_subdirectory().

* Added prefix 'Core' to several tests in
  kokkos/Core/unit_tests/CMakeLists.txt now that prefix is 'Kokkos_'

* Added prefix 'Containers' to several tests in
  kokkos/containers/unit_tests/CMakeLists.txt and
  kokkos/containers/performance_tests/CMakeLists.txt now that prefix is
  'Kokkos_'

* Change name of the kokkos/containers/performance_tests/CMakeLists.txt file
  test 'PerformanceTest_XXX' to 'ContainersPerformanceTest_XXX'.

* Added prefix 'Algorithms' to several tests in
  kokkos/algorithms/unit_tests/CMakeLists.txt now that prefix is 'Kokkos_'

* Removed the usage of tribits_configure_file() and wrapper
  kokkos_configure_file() and just call configure_file().  The location of
  PACKAGE_SORUCE_DIR changed so the calls to tribits_configure_file() no
  longer worked.  (Also, these X_config.h.in files were not using any of the
  TriBITS-supported features that needed the calling of
  tribits_configure_file() so there was no reason to not just call raw
  configure_file().)

SQUASH AGINST: Kokkos: Remove TriBITS subpackages (#11545)

* Fix native build of Kokkos after removing subpackages (trilinos/Trilinos#11545)

This restores the building of the raw CMake build of Kokkos after the
refactoring to remove TriBITS subpackages.

* Kokkos: Remove last of subpackage stuff, fix for tests enable (trilinos/Trilinos#11545)

This gives a full passing build and tests with the Trilinos PR GenConfig
clang-11.0.1 build configuration.

* Fixup update target name in python test script that gets configured

---------

Co-authored-by: Damien L-G <dalg24@gmail.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.

None yet

6 participants