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

add tests in C++ 17, 20, 23 & factorize test.yml #138

Merged
merged 5 commits into from
Jun 21, 2023
Merged

add tests in C++ 17, 20, 23 & factorize test.yml #138

merged 5 commits into from
Jun 21, 2023

Conversation

jbigot
Copy link
Member

@jbigot jbigot commented Jun 20, 2023

No description provided.

@jbigot jbigot force-pushed the update_tests branch 12 times, most recently from e345df1 to 10b0aa1 Compare June 21, 2023 08:15
@jbigot jbigot requested a review from tpadioleau June 21, 2023 11:01
@jbigot jbigot mentioned this pull request Jun 21, 2023
Merged
tpadioleau
tpadioleau previously approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@tpadioleau tpadioleau left a comment

Choose a reason for hiding this comment

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

That seems simpler. Can you clarify that now both images habe the same role ?

include/ddc/chunk_span.hpp Show resolved Hide resolved
docker/test_env/Dockerfile Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@jbigot
Copy link
Member Author

jbigot commented Jun 21, 2023

That seems simpler. Can you clarify that now both images habe the same role ?

what do you mean by that ? They used to and still have similar roles

@tpadioleau
Copy link
Member

I thought one of them was to ensure it works for local development and the other one for GPUs ?
I am not sure anybody is still developing using the distrib packages now.

@jbigot
Copy link
Member Author

jbigot commented Jun 21, 2023 •
edited
Loading

I thought one of them was to ensure it works for local development and the other one for GPUs ? I am not sure anybody is still developing using the distrib packages now.

  • we had focal because we said that was the oldest ubuntu version we supported and hence we tested on it

We need to define the minimum version of the libraries that we support as part of our contract with the users. Saying «whatever version is available on this version of Ubuntu» is a good way to do it.

  • cuda was not available (or not recent enough) on focal, so we tested cuda on jammy

I propose to switch to :
«we test on the latest version of ubuntu and on the oldest we claim to support»
And I propose to switch to jammy as «the oldest version of Ubuntu we claim to support»

@tpadioleau
Copy link
Member

I thought one of them was to ensure it works for local development and the other one for GPUs ? I am not sure anybody is still developing using the distrib packages now.

* we had focal because we said that was the oldest ubuntu version we supported and hence we tested on it

We need to define the minimum version of the libraries that we support as part of our contract with the users. Saying «whatever version is available on this version of Ubuntu» is a good way to do it.

* cuda was not available (or not recent enough) on focal, so we tested cuda on jammy

I propose to switch to : «we test on the latest version of ubuntu and on the oldest we claim to support» And I propose to switch to jammy as «the oldest version of Ubuntu we claim to support»

Yes that works for me.

@jbigot
Copy link
Member Author

jbigot commented Jun 21, 2023

I thought one of them was to ensure it works for local development and the other one for GPUs ? I am not sure anybody is still developing using the distrib packages now.

* we had focal because we said that was the oldest ubuntu version we supported and hence we tested on it

We need to define the minimum version of the libraries that we support as part of our contract with the users. Saying «whatever version is available on this version of Ubuntu» is a good way to do it.

* cuda was not available (or not recent enough) on focal, so we tested cuda on jammy

I propose to switch to : «we test on the latest version of ubuntu and on the oldest we claim to support» And I propose to switch to jammy as «the oldest version of Ubuntu we claim to support»

Yes that works for me.

I would do that in a separate PR. What do you say?

@tpadioleau
Copy link
Member

Yes

@jbigot
Copy link
Member Author

jbigot commented Jun 21, 2023

In that case, as soon as you OK it, I think we can merge

@tpadioleau tpadioleau self-requested a review June 21, 2023 17:57
@jbigot jbigot merged commit f267d5d into main Jun 21, 2023
@jbigot jbigot deleted the update_tests branch June 21, 2023 20:58
blegouix pushed a commit that referenced this pull request Jun 29, 2023
@jbigot @blegouix
add tests in C++ 17, 20, 23 & factorize test.yml (#138)
fa0681a 
* add tests in C++ 17, 20, 23
* factorize test.yml
* test with clang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpadioleau tpadioleau tpadioleau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbigot @tpadioleau