Skip to content

Conversation

@alexbatashev
Copy link
Contributor

Fix issues discovered in #4773

bader
bader previously approved these changes Oct 19, 2021
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

I see multiple failures in LIT tests as well. What is the plan to fix them?


option(XPTI_ENABLE_WERROR OFF)

option(XPTI_BUILD_SAMPLES OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that samples build failed due to -Werror option. Shouldn't we just disable it or there other issues as well?

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'd like -Werror to still be enabled. It sometimes catches bugs. that would've go under radar otherwise. But samples are not installed or tested, so I just excluded them from general builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like -Werror to still be enabled. It sometimes catches bugs. that would've go under radar otherwise. But samples are not installed or tested, so I just excluded them from general builds.

I'm suggesting disabling this option only for samples producing warnings. Excluding them from build can lead to the broken build.

Anyway I don't think XPTI samples are critical, so I'm okay to leave this solution if disabling -Werror for these samples is not easy to implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway I don't think XPTI samples are critical, so I'm okay to leave this solution if disabling -Werror for these samples is not easy to implement.

My intention is to eventually fix warnings properly and enable this option back under --ci-defaults, so that it is built regularly. But for now I just want to unblock my work on #4773

@alexbatashev
Copy link
Contributor Author

I see multiple failures in LIT tests as well. What is the plan to fix them?

I don't see any failures. Was it some sort of infra problem?

@bader
Copy link
Contributor

bader commented Oct 20, 2021

I see multiple failures in LIT tests as well. What is the plan to fix them?

I don't see any failures. Was it some sort of infra problem?

https://github.com/intel/llvm/pull/4773/checks?check_run_id=3925870915

@alexbatashev alexbatashev requested a review from bader October 21, 2021 07:27
@alexbatashev
Copy link
Contributor Author

@andykaylor friendly ping

Copy link
Contributor

@andykaylor andykaylor left a comment

Choose a reason for hiding this comment

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

This looks good as a short term workaround until the warnings are fixed.

@bader bader merged commit 51b450c into intel:sycl Oct 25, 2021
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.

3 participants