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

Fix the msvc failure on json config #1610

Merged
merged 1 commit into from
May 16, 2024
Merged

Fix the msvc failure on json config #1610

merged 1 commit into from
May 16, 2024

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented May 13, 2024

Sorry for another pr on this. I thought the failure from msvc cuda, which is now always failed.
To avoid setting the lib path in msvc, gko makes all test works in the library path (dll location).
I changed the working directory to access testing file in #1607 However, it can not find the dll path now.
follow the matrices/config.hpp.in to pass the file path

@yhmtsai yhmtsai added 1:ST:ready-for-review This PR is ready for review 1:ST:no-changelog-entry Skip the wiki check for changelog update labels May 13, 2024
@yhmtsai yhmtsai added this to the Ginkgo 1.8.0 milestone May 13, 2024
@yhmtsai yhmtsai requested review from MarcelKoch and a team May 13, 2024 20:42
@yhmtsai yhmtsai self-assigned this May 13, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. reg:helper-scripts This issue/PR is related to the helper scripts mainly concerned with development of Ginkgo. labels May 13, 2024
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

LGTM! An alternative (for small files) would be creating a temporary file in the working directory and deleting it afterwards. For the large matrices, it is sensible to store them separately, but for just JSON input, we could do it inline

@yhmtsai yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels May 14, 2024
@yhmtsai
Copy link
Member Author

yhmtsai commented May 14, 2024

I thought the same thing, too. but I also think we can use the filesystem to replace the path when we move to c++17.

@upsj
Copy link
Member

upsj commented May 14, 2024

@yhmtsai std::filesystem is still a bit annoying to use on some slightly older compilers, see https://en.cppreference.com/w/cpp/filesystem

Notes

Using this library may require additional compiler/linker options. GNU implementation prior to 9.1 requires linking with -lstdc++fs and LLVM implementation prior to LLVM 9.0 requires linking with -lc++fs

so I think we better avoid it.

@MarcelKoch MarcelKoch merged commit 3e4f2a9 into develop May 16, 2024
12 of 15 checks passed
@MarcelKoch MarcelKoch deleted the fix_json_msvc branch May 16, 2024 10:37
@ginkgo-bot
Copy link
Member

Error: PR already merged!

Copy link

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:no-changelog-entry Skip the wiki check for changelog update 1:ST:ready-to-merge This PR is ready to merge. 1:ST:skip-full-test reg:build This is related to the build system. reg:helper-scripts This issue/PR is related to the helper scripts mainly concerned with development of Ginkgo. reg:testing This is related to testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants