Skip to content

Conversation

uditagarwal97
Copy link
Contributor

@uditagarwal97 uditagarwal97 commented Oct 17, 2025

Fixes E2E test failure in SeperateCompile/test.cpp in sycl-rel 6.3 ABI testing(https://github.com/intel/llvm/actions/runs/18599730732/job/53035028308#step:27:2964)
The test failure is because of lit using system-installed sycl-post-link instead of the newly built one. This PR forces lit to find and use tools from <compiler build>/bin directory.
These changes are already in sycl branch (brought in by #19669)

@uditagarwal97 uditagarwal97 force-pushed the private/udit/seperate_compiler branch from f580da6 to 9ef13c8 Compare October 19, 2025 18:54
@uditagarwal97 uditagarwal97 changed the base branch from sycl to sycl-rel-6_3 October 19, 2025 18:54
@uditagarwal97 uditagarwal97 changed the title [CI] Add SYCL toolchain to PATH when building E2E tests [E2E][sycl-rel 6.3] Make lit search tools in bin/ directory Oct 19, 2025
@uditagarwal97 uditagarwal97 marked this pull request as ready for review October 19, 2025 19:00
Copy link
Contributor

@KornevNikita KornevNikita left a comment

Choose a reason for hiding this comment

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

LGTM, but why there is no pre-commit run?

@uditagarwal97
Copy link
Contributor Author

LGTM, but why there is no pre-commit run?

Weird. IDK. @AlexeySachkov Do you know what could be going wrong?

@AlexeySachkov
Copy link
Contributor

LGTM, but why there is no pre-commit run?

Weird. IDK. @AlexeySachkov Do you know what could be going wrong?

uditagarwal97 changed the base branch from sycl to sycl-rel-6_3 20 hours ago

This looks suspicious. I wonder if re-opening the PR would help. Worst case scenario re-creating it directly to sycl-rel-6_3 should definitely work.

I don't have fundamental objections against the change, but I would like to see the pre-commit run to confirm that it isn't accidentally broken by this patch (and that the issue with the test is actually resolved)

@uditagarwal97
Copy link
Contributor Author

LGTM, but why there is no pre-commit run?

Weird. IDK. @AlexeySachkov Do you know what could be going wrong?

uditagarwal97 changed the base branch from sycl to sycl-rel-6_3 20 hours ago

This looks suspicious. I wonder if re-opening the PR would help. Worst case scenario re-creating it directly to sycl-rel-6_3 should definitely work.

I don't have fundamental objections against the change, but I would like to see the pre-commit run to confirm that it isn't accidentally broken by this patch (and that the issue with the test is actually resolved)

Thanks, re-opening helped.

@aelovikov-intel
Copy link
Contributor

and that the issue with the test is actually resolved

IIUC, it won't be tested here. Instead, we'd need to merge, re-build the image with the sycl-rel-6.3 pre-compiled E2E binaries and only then have a passing pre-commit job in trunk. @uditagarwal97 can correct me if I'm wrong.

@uditagarwal97
Copy link
Contributor Author

and that the issue with the test is actually resolved

IIUC, it won't be tested here. Instead, we'd need to merge, re-build the image with the sycl-rel-6.3 pre-compiled E2E binaries and only then have a passing pre-commit job in trunk. @uditagarwal97 can correct me if I'm wrong.

@aelovikov-intel I think we should be able to verify the fix using pre-commit. I have another PR targeted to sycl-rel 6.3 (#20390) where all pre-commit E2E jobs failed due to this test failure. Comparing the CI runs in this PR and that in #20390, this PR works as intended and the issue is resolved.

@uditagarwal97
Copy link
Contributor Author

Merging this PR to fix pre-commit - SeperateCompile/test.cpp is failing on every other PR.

@uditagarwal97 uditagarwal97 merged commit 66742c2 into sycl-rel-6_3 Oct 20, 2025
23 checks passed
@uditagarwal97 uditagarwal97 deleted the private/udit/seperate_compiler branch October 20, 2025 19:41
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.

4 participants