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 ability to compile CppInterOp and tests on Windows with different compiler #195

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

mcbarton
Copy link
Collaborator

@mcbarton mcbarton commented Jan 28, 2024

@vgvassilev This PR allows for those trying to solve Windows build issues to use a different compiler to MSVC . With these changes I was able to compile CppInterOp and its tests using the Clang compiler. Currently I'm not passing the tests as the header files cannot be found, but I believe this has something to do with the environment variables I have set, and am investigating.

Fixes #175 (provides proof that earlier PR fixed it)

Copy link

codecov bot commented Jan 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e61e7ca) 78.83% compared to head (30c91dc) 78.83%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #195   +/-   ##
=======================================
  Coverage   78.83%   78.83%           
=======================================
  Files           8        8           
  Lines        3048     3048           
=======================================
  Hits         2403     2403           
  Misses        645      645           
Files Coverage Δ
lib/Interpreter/CppInterOp.cpp 86.31% <ø> (ø)
Files Coverage Δ
lib/Interpreter/CppInterOp.cpp 86.31% <ø> (ø)

Copy link
Contributor

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

@vgvassilev
Copy link
Contributor

Can we use continue-on-error in the yml file instead?

@mcbarton
Copy link
Collaborator Author

mcbarton commented Jan 28, 2024

@vgvassilev So just to be certain before I make the change, you want me to reactivate the other Windows builds, but add continue-on-error: true to the 'Build and Test/Install CppInterOp on Windows systems' part of the jobs?

@vgvassilev
Copy link
Contributor

Yes, assuming we still get some green/ or different than X build sign.

Copy link
Contributor

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

@mcbarton
Copy link
Collaborator Author

mcbarton commented Jan 28, 2024

@vgvassilev After that change the Windows builds now get a green tick. I have added a comment to this section that tests are expected to fail until #188 is resolved.

@mcbarton
Copy link
Collaborator Author

@vgvassilev Can this PR be merged now that is gets a green tick for all the jobs?

Copy link
Contributor

github-actions bot commented Feb 2, 2024

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

1 similar comment
Copy link
Contributor

github-actions bot commented Feb 2, 2024

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

Copy link
Contributor

github-actions bot commented Feb 2, 2024

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

@mcbarton
Copy link
Collaborator Author

mcbarton commented Feb 2, 2024

@alexander-penev can you review this PR for me? The purpose of the PR is to do 3 things

  1. Enable the ability to build CppInterOp on Windows with a different compiler to MSVC
  2. Add 'continue on error' for CppInterOp build on Windows until codebase can be changed for tests to pass
  3. Add osx 14 + osx arm build to CI (cc: @vgvassilev new addition)

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! Thank you!

@vgvassilev vgvassilev merged commit 5953058 into compiler-research:main Feb 2, 2024
34 checks passed
@mcbarton mcbarton deleted the windows branch February 12, 2024 19:29
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.

Clang-repl=ON issue for both clang 16 & 17 on Apple Silicon for cppyy
2 participants