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

Fixes and updates, bump third party libraries #1619

Merged
merged 13 commits into from
Jun 6, 2024
Merged

Conversation

tcojean
Copy link
Member

@tcojean tcojean commented May 29, 2024

This PR fixes some build and CI issues, also addresses some problems reported by users, and other release preparation aspects.

  • bump third party libraries
  • osx: try to also install (and run with) open-mpi in CI (disabled later, but works)
  • osx: try to help cmake find libomp (currently not found in MacOS CI!)
  • fix tests' gtest helpers: Pass OpenMP when required
  • Fix: Cast according to Issue Fail to build on 32-bit x86 windows #1562
  • benchmark: clarify manual vs helper script
  • build[msvc]: change the library structure to workaround the symbol limit
  • ci[msvc,cuda]: use windows 2019, which somehow works (windows-latest doesn't)
  • ci: workaround sonarqube issues on horeka

In some cases, the commit message has more details.

Closes issues #1481 #1482 #1141 #1562

@ginkgo-bot ginkgo-bot added the reg:ci-cd This is related to the continuous integration system. label May 29, 2024
@tcojean tcojean force-pushed the fixes_and_check branch 2 times, most recently from 7527266 to db63a9a Compare May 29, 2024 14:38
@tcojean tcojean force-pushed the fixes_and_check branch 2 times, most recently from 6d7124f to 1a107e2 Compare May 31, 2024 08:54
@tcojean
Copy link
Member Author

tcojean commented May 31, 2024

Small note that MPI seems to work with these MacOS runners with small changes, see: https://github.com/ginkgo-project/ginkgo/actions/runs/9290681420/job/25567479064?pr=1619 (only the benchmark tests fail because of a write issue which changes stdout/stderr).

  • I will disable MPI nonetheless because some test take a long time on these VM, and there is a risk of reaching the timeout.
  • I will keep the fixed OpenMP MacOS CI, as that was previously not tested.

See failure:
https://gitlab.com/ginkgo-project/ginkgo-public-ci/-/jobs/6963993256#L654
and many others.

It seems that HoreKa and NodeJS do not go well together.
Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.03%. Comparing base (d7bc39e) to head (06868cb).
Report is 45 commits behind head on develop.

Current head 06868cb differs from pull request most recent head 43189ba

Please upload reports for the commit 43189ba to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1619      +/-   ##
===========================================
+ Coverage    89.28%   90.03%   +0.75%     
===========================================
  Files          752      758       +6     
  Lines        60467    61154     +687     
===========================================
+ Hits         53985    55058    +1073     
+ Misses        6482     6096     -386     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tcojean tcojean changed the title Test please ignore: checking errors reported in old issues Fixes and updates, bump third party libraries Jun 5, 2024
@tcojean tcojean requested review from a team June 5, 2024 09:01
@tcojean
Copy link
Member Author

tcojean commented Jun 5, 2024

Note that there is still a failure with the warnings job which is annoying: https://gitlab.com/ginkgo-project/ginkgo-public-ci/-/jobs/7016226162

A workaround for the issue is to add the C language to CMake, which allows CMake to properly find pthread. I believe more recent versions of CMake reviewed the FindThreads module which should also fix the issue.

My intuition is there is something wrong with the docker container, as I cannot seem to reproduce this easily. If anyone has an idea, I would be happy to test it.

BENCHMARKING.md Outdated Show resolved Hide resolved
@@ -29,7 +29,7 @@ jobs:
config:
- {version: "latest", name: "cuda-latest/release/shared", "mixed": "ON"}
name: msvc/${{ matrix.config.name }} (only compile)
runs-on: [windows-latest]
runs-on: [windows-2019]
Copy link
Member

Choose a reason for hiding this comment

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

I guess a more permanent fix would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, but I've seen other pipelines fail with the same problem, e.g. https://github.com/Jimver/cuda-toolkit/actions/runs/8767123552/job/24060026380#step:11:101 so there is something fishy going on with Github's Windows setup in general.

@MarcelKoch
Copy link
Member

Regarding the warnings CI fail, I would prefer if we don't enable the C language. I would be fine with this job failing for a while until we have a proper fix.

.gitlab-ci.yml Show resolved Hide resolved
BENCHMARKING.md Outdated Show resolved Hide resolved
BENCHMARKING.md Outdated Show resolved Hide resolved
BENCHMARKING.md Outdated Show resolved Hide resolved
BENCHMARKING.md Outdated Show resolved Hide resolved
core/test/gtest/CMakeLists.txt Outdated Show resolved Hide resolved
core/test/gtest/CMakeLists.txt Outdated Show resolved Hide resolved
${add_test_MPI_SIZE}
"$<TARGET_FILE:${test_target_name}>"
WORKING_DIRECTORY "$<TARGET_FILE_DIR:ginkgo>")
if (add_test_MPI_SIZE LESS_EQUAL MPIEXEC_MAX_NUMPROCS)
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary? MPIEXEC_MAX_NUMPROCS is the number of processor on the host. but mpi should be able run more than it, right?

Copy link
Member Author

@tcojean tcojean Jun 5, 2024

Choose a reason for hiding this comment

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

See this job for example https://github.com/ginkgo-project/ginkgo/actions/runs/9288144680/job/25558834125#step:6:2170

There are other ways to fix this, such as allowing oversubscription, but this could make these already slow tests take a really long time, which I would vote against.

Co-authored-by: Yu-Hsiang Tsai <[email protected]>
Co-authored-by: Marcel Koch <[email protected]>
Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

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

LGTM, mostly nits.

BENCHMARKING.md Outdated Show resolved Hide resolved
BENCHMARKING.md Outdated Show resolved Hide resolved
BENCHMARKING.md Outdated Show resolved Hide resolved
BENCHMARKING.md Outdated Show resolved Hide resolved
BENCHMARKING.md Outdated Show resolved Hide resolved
@tcojean tcojean self-assigned this Jun 5, 2024
@tcojean tcojean added this to the Ginkgo 1.8.0 milestone Jun 5, 2024
@tcojean
Copy link
Member Author

tcojean commented Jun 6, 2024

Any other comments? Otherwise I'll merge this soon.

@tcojean tcojean merged commit 5fa369a into develop Jun 6, 2024
14 of 15 checks passed
@tcojean tcojean deleted the fixes_and_check branch June 6, 2024 09:47
Copy link

sonarcloud bot commented Jun 6, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:run-full-test reg:ci-cd This is related to the continuous integration system.
Projects
None yet
5 participants