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

Add workflow to test examples are compilable #53

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

d-alonso
Copy link
Contributor

@d-alonso d-alonso commented Dec 9, 2024

And fix those codes that trip the new workflow

Example of a failing run: https://github.com/codee-com/open-catalog/actions/runs/12238572380/job/34136975245

@d-alonso d-alonso self-assigned this Dec 9, 2024
@d-alonso d-alonso force-pushed the feature/fixNonCompilableOMP branch 4 times, most recently from 16fc2b8 to a7f4bee Compare December 9, 2024 15:16
@d-alonso d-alonso requested review from a team, jgonzac, inaki-amatria, daniel-otero and alvrogd and removed request for a team December 9, 2024 15:17
Copy link
Contributor

@jgonzac jgonzac left a comment

Choose a reason for hiding this comment

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

Smart 👍🏼

Copy link
Collaborator

@alvrogd alvrogd left a comment

Choose a reason for hiding this comment

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

Looking good! This will be super-helpful :)

.github/utils/CheckExamples/CMakeLists.txt Outdated Show resolved Hide resolved
.github/utils/CheckExamples/CMakeLists.txt Outdated Show resolved Hide resolved
.github/utils/CheckExamples/CMakeLists.txt Outdated Show resolved Hide resolved
@alvrogd
Copy link
Collaborator

alvrogd commented Dec 9, 2024

Just noticed that the output of the script you shared seems to be a little convoluted. For instance, the first failure occurs when compiling PWD003/example-omp.f90, and the corresponding compiler errors are shown after the first status line of the next file (Trying ... PWD004 ...).

Could we solve this by only keeping the last status line for each file?; i.e., the ones with the Good or Bad note.

@d-alonso
Copy link
Contributor Author

d-alonso commented Dec 10, 2024

Just noticed that the output of the script you shared seems to be a little convoluted. For instance, the first failure occurs when compiling PWD003/example-omp.f90, and the corresponding compiler errors are shown after the first status line of the next file (Trying ... PWD004 ...).

Could we solve this by only keeping the last status line for each file?; i.e., the ones with the Good or Bad note.

This was easier than expected, I just used the CHECK_{START,PASS,FAIL} that CMake already provides. I think it's good to keep the two lines in case the try_compile call has a CMake error itself, you'll see which file was going to be compiled right above it (I know because it happened to me).

@d-alonso d-alonso force-pushed the feature/fixNonCompilableOMP branch 2 times, most recently from 0182eaa to a7432b1 Compare December 10, 2024 08:52
@d-alonso d-alonso requested a review from alvrogd December 10, 2024 09:42
Copy link
Collaborator

@alvrogd alvrogd left a comment

Choose a reason for hiding this comment

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

This was easier than expected, I just used the CHECK_{START,PASS,FAIL} that CMake already provides. I think it's good to keep the two lines in case the try_compile call has a CMake error itself, you'll see which file was going to be compiled right above it (I know because it happened to me).

That's great! This approach looks good to me.

They contained some wrong directive clauses that either
- Referenced not-yet-declared variables
- Were missing the datascoping for a referenced variable
@d-alonso d-alonso force-pushed the feature/fixNonCompilableOMP branch from a7432b1 to 6ceccc3 Compare December 10, 2024 15:41
@d-alonso d-alonso merged commit 6ceccc3 into main Dec 10, 2024
7 checks passed
@d-alonso d-alonso deleted the feature/fixNonCompilableOMP branch December 10, 2024 15:48
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