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

[SYCL] Add clang-nvlink-wrapper to sycl-toolchain as a dependency #17035

Conversation

omarahmed1111
Copy link
Contributor

This is a small follow-up PR on this PR as clang-nvlink-wrapper tool need to be added as a dependency for sycl-toolchain.

@dm-vodopyanov
Copy link
Contributor

dm-vodopyanov commented Feb 21, 2025

@omarahmed1111, please, follow rules of adding tags to captions for all your PRs: https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ContributeToDPCPP.md#commit-message

@intel/llvm-gatekeepers, please, don't forget about any of these requirements, when you merge PRs.

@martygrant
Copy link
Contributor

@dm-vodopyanov is this something we could handle automatically with labels? We done this in the UR repo https://github.com/oneapi-src/unified-runtime/blob/main/.github/labeler.yml although that would specifically only be on PRs visible on Github, not the commits themselves

@omarahmed1111 omarahmed1111 changed the title Add clang-nvlink-wrapper to sycl-toolchain as a dependency [SYCL] Add clang-nvlink-wrapper to sycl-toolchain as a dependency Feb 21, 2025
@omarahmed1111 omarahmed1111 force-pushed the add-clang-nvlink-wrapper-to-sycl-toolchain branch from 5c52aa5 to 38bc8f9 Compare February 21, 2025 14:40
@aelovikov-intel
Copy link
Contributor

@dm-vodopyanov is this something we could handle automatically with labels? We done this in the UR repo https://github.com/oneapi-src/unified-runtime/blob/main/.github/labeler.yml although that would specifically only be on PRs visible on Github, not the commits themselves

I think part of the idea was to align with upstream llvm in how we title our commits. + @bader

@dm-vodopyanov
Copy link
Contributor

dm-vodopyanov commented Feb 21, 2025

@dm-vodopyanov is this something we could handle automatically with labels? We done this in the UR repo https://github.com/oneapi-src/unified-runtime/blob/main/.github/labeler.yml although that would specifically only be on PRs visible on Github, not the commits themselves

I think part of the idea was to align with upstream llvm in how we title our commits. + @bader

+ another part which was came out after the idea above- use them during writing the Release Notes - easier to see what we need to include and what we don't

@dm-vodopyanov is this something we could handle automatically with labels?

Of course we can consider some changes in the future, but currently we should stick to what we have now: these requirements in the doc.

@aelovikov-intel
Copy link
Contributor

I also think that relying on a particular hosting/code review system is wrong as this is much more likely to change than version control system. I'd rather have information at the most stable layer (e.g., tags in commit messages, full description in the commit message instead of just link to some issue, etc.)

@omarahmed1111
Copy link
Contributor Author

@intel/llvm-gatekeepers Please merge, Thanks!

@dm-vodopyanov dm-vodopyanov merged commit 2ed0eba into intel:sycl Feb 21, 2025
19 checks passed
@bader
Copy link
Contributor

bader commented Feb 21, 2025

@aelovikov-intel is right. The ultimate goal for SYCL project is to be upstreamed to llvm-project. To make the upstream process lighter we adopted LLVM contribution practices (see our contribution guide https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md#contribution-process). Adding tags to the commit message is recommended by LLVM's Developer Policy:

When the changes are restricted to a specific part of the code (e.g. a back-end or optimization pass), it is customary to add a tag to the beginning of the line in square brackets. For example, “[SCEV] …” or “[OpenMP] …”. This helps email filters and searches for post-commit reviews.

I see how it can be annoying to add the same tag to almost all your patches when you contribute mostly to a single component. Adding [UR] tag can be automated via:

  • git commit message template
  • git commit-msg hook
  • GitHub Action job modifying PR title/description.

I suppose there are other options to make automate tagging, but these are the first things come to my mind. I'm all for automating such tasks.

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