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

Remove continue-on-error Windows jobs ci #313

Merged

Conversation

mcbarton
Copy link
Collaborator

@mcbarton mcbarton commented Aug 3, 2024

The objective of this PR is to add to the ci a non wasm build of xeus-cpp which runs its various tests. This should allow debugging of the missing symbols needed to make Windows work (discussion here compiler-research/xeus-cpp#144).

Copy link

codecov bot commented Aug 3, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.29%. Comparing base (920347b) to head (0f4f348).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/Interpreter/Paths.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #313      +/-   ##
==========================================
+ Coverage   74.27%   74.29%   +0.01%     
==========================================
  Files           8        8              
  Lines        3188     3190       +2     
==========================================
+ Hits         2368     2370       +2     
  Misses        820      820              
Files with missing lines Coverage Δ
lib/Interpreter/Compatibility.h 89.90% <100.00%> (+0.18%) ⬆️
lib/Interpreter/CppInterOp.cpp 79.83% <ø> (ø)
lib/Interpreter/DynamicLibraryManager.cpp 73.46% <100.00%> (ø)
lib/Interpreter/Paths.cpp 37.74% <0.00%> (ø)
Files with missing lines Coverage Δ
lib/Interpreter/Compatibility.h 89.90% <100.00%> (+0.18%) ⬆️
lib/Interpreter/CppInterOp.cpp 79.83% <ø> (ø)
lib/Interpreter/DynamicLibraryManager.cpp 73.46% <100.00%> (ø)
lib/Interpreter/Paths.cpp 37.74% <0.00%> (ø)

@mcbarton mcbarton force-pushed the Add-non-wasm-build-xeus-cpp branch 3 times, most recently from aec490f to c879a57 Compare August 7, 2024 10:35
@mcbarton
Copy link
Collaborator Author

mcbarton commented Aug 8, 2024

@vgvassilev looking at the output of the ci here https://github.com/compiler-research/CppInterOp/actions/runs/10288514553/job/28502994037 it appears that only a few symbols are left to be exported to get std::cout and std::cerr to work in xeus-cpp on Windows, but I can't work out where they need to be exported, since both CppInterOp and xeus-cpp are exporting the symbols it is complaining about. Any idea what these symbols need to be exported?

@mcbarton mcbarton force-pushed the Add-non-wasm-build-xeus-cpp branch 5 times, most recently from f640d31 to e507f53 Compare August 17, 2024 20:20
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

lib/Interpreter/DynamicLibraryManager.cpp Outdated Show resolved Hide resolved
@mcbarton mcbarton force-pushed the Add-non-wasm-build-xeus-cpp branch from 24ab8b9 to de94995 Compare August 21, 2024 12:33
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

lib/Interpreter/DynamicLibraryManager.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

lib/Interpreter/DynamicLibraryManager.cpp Outdated Show resolved Hide resolved
lib/Interpreter/DynamicLibraryManager.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

lib/Interpreter/Paths.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved
lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved
lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

3 similar comments
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@mcbarton mcbarton force-pushed the Add-non-wasm-build-xeus-cpp branch from 039bf03 to 8b248a3 Compare August 21, 2024 19:26
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

4 similar comments
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@mcbarton mcbarton force-pushed the Add-non-wasm-build-xeus-cpp branch 3 times, most recently from bfc0f56 to 14b0a6a Compare October 25, 2024 19:58
@mcbarton mcbarton changed the title Get Windows jobs in ci to pass. Remove continue-on-error Windows jobs ci Oct 25, 2024
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@mcbarton mcbarton marked this pull request as ready for review October 25, 2024 20:32
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/CppInterOp/CppInterOpConfig.cmake.in Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/modules/GoogleTest.cmake Outdated Show resolved Hide resolved
cmake/modules/GoogleTest.cmake Outdated Show resolved Hide resolved
lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved
lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved
lib/Interpreter/DynamicLibraryManager.cpp Outdated Show resolved Hide resolved
unittests/CppInterOp/CUDATest.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@mcbarton mcbarton force-pushed the Add-non-wasm-build-xeus-cpp branch 2 times, most recently from a7093c3 to a256714 Compare October 26, 2024 13:02
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@mcbarton mcbarton force-pushed the Add-non-wasm-build-xeus-cpp branch 4 times, most recently from 8400eb5 to 9b86ee2 Compare October 26, 2024 15:06
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Lgtm modulo the comment.

@mcbarton
Copy link
Collaborator Author

@vgvassilev getenv_s is not being recognised on Unix systems. Any ideas why?

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

lib/Interpreter/DynamicLibraryManager.cpp Outdated Show resolved Hide resolved
lib/Interpreter/DynamicLibraryManager.cpp Outdated Show resolved Hide resolved
lib/Interpreter/Paths.cpp Outdated Show resolved Hide resolved
lib/Interpreter/Paths.cpp Outdated Show resolved Hide resolved
lib/Interpreter/Paths.cpp Show resolved Hide resolved
@mcbarton mcbarton force-pushed the Add-non-wasm-build-xeus-cpp branch 5 times, most recently from 0f1a770 to da0902c Compare October 26, 2024 18:16
@mcbarton mcbarton force-pushed the Add-non-wasm-build-xeus-cpp branch from da0902c to 0f4f348 Compare October 26, 2024 18:17
@mcbarton mcbarton requested a review from vgvassilev October 26, 2024 18:54
@vgvassilev vgvassilev merged commit c0a4dfe into compiler-research:main Oct 26, 2024
54 of 55 checks passed
@mcbarton mcbarton mentioned this pull request Oct 27, 2024
@Gnimuc
Copy link
Contributor

Gnimuc commented Jan 11, 2025

@vgvassilev getenv_s is not being recognised on Unix systems. Any ideas why?

looks like getenv_s is optional in the C11 standard.

X-ref: https://stackoverflow.com/questions/372980/do-you-use-the-tr-24731-safe-functions/373911#373911

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.

4 participants