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

use specific commit of amalgamate for single-header branch #2437

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

mehmetyusufoglu
Copy link
Contributor

@mehmetyusufoglu mehmetyusufoglu commented Dec 6, 2024

This is a solution to issue #2431

Problem [Updated]

  • The commit a904e3255cf05a92d32036e636e21378a2681071 (of amalgamate repo) starts looking for include file paths which does not include underschore charecter, If there is an "_" the file is not detected as an #inlude line and cannot be embedded to the resulting single header
    #include "alpaka/queue/cuda_hip/QueueUniformCudaHipRt.hpp"

Solution

  • The directory name "alpaka/queue/cuda_hip" is changed with "alpaka/queue/cuda-hip", hence correspoding #include lines are also changed.
  • Since we don't use CamelCase in directory names, or no capital letters is used this seems to be a resonable solution
  • [Added on Jan 6th] There was a bug at .clang-format file. The character "-" in file-path regex must be escaped since it is used in definition of a Range like in a-z. Otherwise the include directive having a file-path including "-" will be listed separately at the end.

@mehmetyusufoglu mehmetyusufoglu marked this pull request as draft December 6, 2024 13:29
@mehmetyusufoglu
Copy link
Contributor Author

mehmetyusufoglu commented Dec 11, 2024

@psychocoderHPC

There are some files which has #include directive inside #ifdef directive like the file alpaka/include/alpaka/kernel/TaskKernelGpuUniformCudaHipRt.hpp. Did you mean those files could be reason of the problem?


#if defined(ALPAKA_ACC_GPU_CUDA_ENABLED) || defined(ALPAKA_ACC_GPU_HIP_ENABLED)

#    if !defined(ALPAKA_HOST_ONLY)

#        include "alpaka/core/BoostPredef.hpp"

#        if defined(ALPAKA_ACC_GPU_CUDA_ENABLED) && !BOOST_LANG_CUDA
#            error If ALPAKA_ACC_GPU_CUDA_ENABLED is set, the compiler has to support CUDA!
#        endif

@SimeonEhrig
Copy link
Member

  • The latest working commit of amalgamate repo is from July 17, 2024. Therefore it is used
    a4796f0515fbc56d375a5013dd9004605df9c761

@mehmetyusufoglu Do you know, why it is not working anymore? I checked the next commit and it is only adding test cases. Therefore it should also work.

In general it is better to find out what was changed in the amalgamate script and fix our code than sticking on a old commit. If it to complicated to fix, we can stick on a specific commit hash.

@mehmetyusufoglu
Copy link
Contributor Author

  • The latest working commit of amalgamate repo is from July 17, 2024. Therefore it is used
    a4796f0515fbc56d375a5013dd9004605df9c761

@mehmetyusufoglu Do you know, why it is not working anymore? I checked the next commit and it is only adding test cases. Therefore it should also work.

In general it is better to find out what was changed in the amalgamate script and fix our code than sticking on a old commit. If it to complicated to fix, we can stick on a specific commit hash.

The error is introduced by a904e3255cf05a92d32036e636e21378a2681071, the second after the one i used to go back to solve the problem. It is quite long.

I am trying to understand what is the reason introduced by this commit.

@SimeonEhrig
Copy link
Member

  • The latest working commit of amalgamate repo is from July 17, 2024. Therefore it is used
    a4796f0515fbc56d375a5013dd9004605df9c761

@mehmetyusufoglu Do you know, why it is not working anymore? I checked the next commit and it is only adding test cases. Therefore it should also work.
In general it is better to find out what was changed in the amalgamate script and fix our code than sticking on a old commit. If it to complicated to fix, we can stick on a specific commit hash.

The error is introduced by a904e3255cf05a92d32036e636e21378a2681071, the second after the one i used to go back to solve the problem. It is quite long.

I am trying to understand what is the reason introduced by this commit.

I was already in fear that this PR introduced the problem :-(

@mehmetyusufoglu
Copy link
Contributor Author

mehmetyusufoglu commented Dec 17, 2024

  • The latest working commit of amalgamate repo is from July 17, 2024. Therefore it is used
    a4796f0515fbc56d375a5013dd9004605df9c761

@mehmetyusufoglu Do you know, why it is not working anymore? I checked the next commit and it is only adding test cases. Therefore it should also work.
In general it is better to find out what was changed in the amalgamate script and fix our code than sticking on a old commit. If it to complicated to fix, we can stick on a specific commit hash.

The error is introduced by a904e3255cf05a92d32036e636e21378a2681071, the second after the one i used to go back to solve the problem. It is quite long.
I am trying to understand what is the reason introduced by this commit.

I was already in fear that this PR introduced the problem :-(

Ok, they restricted the #included file name search and file paths which includes "_" can not be detected. Hence #include file path which consists "_" can not found. #include "alpaka/queue/cuda_hip/QueueUniformCudaHipRt.hpp

//change at amalgamate that restrict include path of included file
-       [[ $line =~ ^[\ \       ]*\#[\ \t]*include[\ \t]*\".*\".* ]]
+       [[ $line =~ ^[\ \       ]*\#[\ \        ]*include[\ \   ]*\"[A-Za-z0-9\+\.\/\-]+\".* ]]

@mehmetyusufoglu mehmetyusufoglu force-pushed the fixSingleHeader branch 2 times, most recently from 632fb95 to 8430f8b Compare December 19, 2024 09:23
@mehmetyusufoglu mehmetyusufoglu marked this pull request as ready for review December 19, 2024 09:23
@psychocoderHPC psychocoderHPC added this to the 2.0.0 milestone Dec 19, 2024
SimeonEhrig
SimeonEhrig previously approved these changes Jan 6, 2025
@SimeonEhrig
Copy link
Member

Looks good. I think we can merge after PR #2442 which fixes the CI.

@mehmetyusufoglu
Copy link
Contributor Author

If it is ready someone can merge [ I am on vacation during 6-10 January]

#include "alpaka/workdiv/WorkDivHelpers.hpp"
#include "alpaka/workdiv/WorkDivMembers.hpp"

#include <stdexcept>
#include <tuple>
#include <type_traits>

#include "alpaka/queue/cuda-hip/QueueUniformCudaHipRt.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

why move this here instead of staying above with the other alpaka headers ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clang-format-16 changed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

clang-format-16 changed it.

Because there is a bug in the regular expressions used in .clang-format:

diff --git a/.clang-format b/.clang-format
index acd9fbd3b37..dccbb183c20 100644
--- a/.clang-format
+++ b/.clang-format
@@ -135,20 +135,20 @@ UseTab: Never
 #IfMacros: []
 IncludeCategories:
   # Local headers (in "") above all else
-  - Regex: '"([A-Za-z0-9.\/-_])+"'
+  - Regex: '"([A-Za-z0-9.\/_-])+"'
     Priority: 1
   # "alpaka/foo.hpp" after local headers (occur inside alpaka)
-  - Regex: '"alpaka/([A-Za-z0-9.\/-_])+"'
+  - Regex: '"alpaka/([A-Za-z0-9.\/_-])+"'
     Priority: 2
   # <alpaka/foo.hpp> after local headers (occur outside alpaka in examples and test)
-  - Regex: '<alpaka/([A-Za-z0-9.\/-_])+>'
+  - Regex: '<alpaka/([A-Za-z0-9.\/_-])+>'
     Priority: 3
   # C++ standard library headers are the last group to be included
-  - Regex: '<([A-Za-z0-9\/-_])+>'
+  - Regex: '<([A-Za-z0-9\/_-])+>'
     Priority: 5
   # Includes that made it this far are third-party headers and will be placed
   # below alpaka's includes
-  - Regex: '<([A-Za-z0-9.\/-_])+>'
+  - Regex: '<([A-Za-z0-9.\/_-])+>'
     Priority: 4
 # Macros: []
 # NamespaceMacros: []

Copy link
Contributor Author

@mehmetyusufoglu mehmetyusufoglu Jan 6, 2025

Choose a reason for hiding this comment

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

Yes I see, thanks. This is not deliberate a bug as you said. "-" should be escaped otherwise it is a Range defining character.

Corrected by escaping "-" and putting it to the end of the regex. ( escaping "/" is not needed removed)


-  - Regex: '"([A-Za-z0-9.\/-_])+"'
+  - Regex: '"([A-Za-z0-9./_\-])+"'
     Priority: 1
-  # "alpaka/foo.hpp" after local headers (occur inside alpaka)
-  - Regex: '"alpaka/([A-Za-z0-9.\/-_])+"'
+  # "alpaka/foo.hpp" after local headers (occur inside alpaka)  
+  - Regex: '"alpaka/([A-Za-z0-9./_\-])+"'
     Priority: 2
   # <alpaka/foo.hpp> after local headers (occur outside alpaka in examples and test)
-  - Regex: '<alpaka/([A-Za-z0-9.\/-_])+>'
+  - Regex: '<alpaka/([A-Za-z0-9./_\-])+>'
     Priority: 3
   # C++ standard library headers are the last group to be included
-  - Regex: '<([A-Za-z0-9\/-_])+>'
+  - Regex: '<([A-Za-z0-9/_\-])+>'
     Priority: 5
   # Includes that made it this far are third-party headers and will be placed
-  # below alpaka's includes
-  - Regex: '<([A-Za-z0-9.\/-_])+>'
+  # below alpaka's includes 
+  - Regex: '<([A-Za-z0-9./_\-])+>'

I have checked all hpp and cpp files with the new format file, it did not changed anything. And corrected deliberately made errors properly.

find . -type f -name "*.hpp" | grep -v ^"./thirdParty" | xargs clang-format-16 -i
find . -type f -name "*.cpp" | grep -v ^"./thirdParty" | xargs clang-format-16 -i

.clang-format Outdated Show resolved Hide resolved
@SimeonEhrig SimeonEhrig merged commit 2ea8055 into alpaka-group:develop Jan 15, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants