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.

Sign up for GitHub

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

Jump to bottom

ref: Update CI and allow for c++20 #128

Merged
merged 1 commit into from
Nov 19, 2024
Merged

ref: Update CI and allow for c++20 #128

merged 1 commit into from
Nov 19, 2024

Conversation

niermann999
Copy link
Contributor

@niermann999 niermann999 commented Nov 12, 2024 •
edited
Loading

Update github actions and switch to newer containers. This PR is mainly meant to switch on c++20 for algebra-plugins. Due to a bug in the oneAPI compiler, several of the SYCL tests had to be disabled. Furthermore, the Vc test had to be disabled for MSVC due to compilation problems in Vc itself.

stephenswat
stephenswat approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

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

LGTM!

@niermann999 niermann999 force-pushed the ref-CI branch 2 times, most recently from 9d6d132 to e8a6733 Compare November 12, 2024 13:02
krasznaa
krasznaa reviewed Nov 12, 2024
View reviewed changes
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Is there still an issue on MSVC that I should look at?

.github/ci_setup.sh Outdated Show resolved Hide resolved
@niermann999 niermann999 force-pushed the ref-CI branch 2 times, most recently from 19453e2 to 526b28b Compare November 12, 2024 13:13
@niermann999 niermann999 changed the title ref: Update CI ref: Update CI and allow for c++20 Nov 12, 2024
@niermann999 niermann999 added the enhancement New feature or request label Nov 12, 2024
@niermann999 niermann999 force-pushed the ref-CI branch 2 times, most recently from ec6759f to dc5aa61 Compare November 12, 2024 14:02
@krasznaa
Copy link
Member

Unfortunately Vc is beyond repair... 😦 Just turn it off in the Windows CI build. (-DALGEBRA_PLUGINS_SETUP_VC=FALSE -DALGEBRA_PLUGINS_INCLUDE_VC=FALSE with some YAML logic...)

I also see an error coming from Eigen on Windows, while building the CUDA tests. Which don't actually look like an MSVC bug, but rather an Eigen one. But since the CI doesn't test CUDA on Windows, let's just ignore that one for now... 🤔

@niermann999
Copy link
Contributor Author

MSVC seems fine now, but I see lot's of segfaults in the SYCL tests....

@niermann999 niermann999 added the help wanted Extra attention is needed label Nov 13, 2024
@niermann999 niermann999 force-pushed the ref-CI branch 10 times, most recently from 65c022f to 72c6f60 Compare November 14, 2024 08:07
@niermann999
Copy link
Contributor Author

I think the sycl problem here is the same as acts-project/traccc#655, so I have disabled the failing tests for now

@niermann999 niermann999 requested a review from krasznaa November 14, 2024 08:09
This was referenced Nov 14, 2024
Merged
Closed
krasznaa
krasznaa reviewed Nov 18, 2024
View reviewed changes
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I think I'll use this repo to get help from the Intel developers for our issues. So I'm very much on board with the C++20 upgrade. I just don't want to turn off more than absolutely needed. 🤔

.github/ci_setup.sh Show resolved Hide resolved
.github/workflows/builds.yml Show resolved Hide resolved
tests/accelerator/sycl/common/test_sycl_basics.hpp Outdated Show resolved Hide resolved
@niermann999 niermann999 force-pushed the ref-CI branch 2 times, most recently from 6098289 to 6fe81d8 Compare November 18, 2024 16:31
@niermann999
Copy link
Contributor Author

Also updated the vecmem version now, since it was before only done on one of the dependent PRs

krasznaa
krasznaa reviewed Nov 18, 2024
View reviewed changes
.github/ci_setup.sh Show resolved Hide resolved
.github/workflows/builds.yml Outdated Show resolved Hide resolved
.github/workflows/builds.yml Outdated Show resolved Hide resolved
@niermann999 niermann999 force-pushed the ref-CI branch 2 times, most recently from 9e4847a to 5f0aa23 Compare November 18, 2024 18:30
@niermann999 niermann999 force-pushed the ref-CI branch 2 times, most recently from 8280c3e to ab97d2f Compare November 19, 2024 10:25
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@niermann999
Update github actions and switch to newer containers.
ab97d2f 
This PR is mainly meant to switch on c++20 for algebra-plugins.
Due to a bug in the oneAPI compiler, several of the SYCL tests
had to be disabled. Furthermore, the Vc test had to be disabled
for MSVC due to compilation issues in Vc itself.
@krasznaa krasznaa mentioned this pull request Nov 19, 2024
Merged
krasznaa
krasznaa approved these changes Nov 19, 2024
View reviewed changes
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Fine, let's finally go ahead with it. I'd also like to move on with further debugging the SYCL issues...

@niermann999
Copy link
Contributor Author

I can't merge this anymore. Can somebody hit the button for me @krasznaa @paulgessinger ?

@krasznaa
Copy link
Member

I can't merge this anymore. Can somebody hit the button for me @krasznaa @paulgessinger ?

Unfortunately in yesterday's cleanup I also lost my privileges on the repository. 😦

image

@paulgessinger
Copy link
Member

Whoops, let me check what's going on.

@paulgessinger
Copy link
Member

@krasznaa @niermann999 can you try again now?

@krasznaa
Copy link
Member

Yepp. Looks better now. 😉

image

@krasznaa krasznaa merged commit 6866c1b into acts-project:main Nov 19, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krasznaa krasznaa krasznaa approved these changes

@stephenswat stephenswat stephenswat approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@niermann999 @krasznaa @paulgessinger @stephenswat