Skip to content

Conversation

@aaronj0
Copy link
Collaborator

@aaronj0 aaronj0 commented Feb 18, 2025

Prefix USE_REPL and USE_CLING

@aaronj0 aaronj0 requested a review from vgvassilev February 18, 2025 08:12
@mcbarton
Copy link
Collaborator

mcbarton commented Feb 18, 2025

@aaronj0 I don't understand the motivation for this change. Seems random. The PR description doesn't say why it needs to be done, just what is being done.

cc @vgvassilev

Also the workflows (and probably the code too even though I haven't checked) in cppyy and related repos all assume the old options, so will break a lot of things. If any other repo which builds on CppInterOp we don't work on is using the old options they will break too.

@codecov
Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.29%. Comparing base (be5141c) to head (c562c1c).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #505   +/-   ##
=======================================
  Coverage   71.29%   71.29%           
=======================================
  Files           9        9           
  Lines        3567     3567           
=======================================
  Hits         2543     2543           
  Misses       1024     1024           
Files with missing lines Coverage Δ
lib/Interpreter/CXCppInterOp.cpp 22.09% <ø> (ø)
lib/Interpreter/Compatibility.h 88.28% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 80.60% <ø> (ø)
Files with missing lines Coverage Δ
lib/Interpreter/CXCppInterOp.cpp 22.09% <ø> (ø)
lib/Interpreter/Compatibility.h 88.28% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 80.60% <ø> (ø)

@github-actions
Copy link
Contributor

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

@aaronj0
Copy link
Collaborator Author

aaronj0 commented Feb 18, 2025

@aaronj0 I don't understand the motivation for this change. Seems random. The PR description doesn't say why it needs to be done, just what is being done.

cc @vgvassilev

Also the workflows (and probably the code too even though I haven't checked) in cppyy and related repos all assume the old options, so will break a lot of things. If any other repo which builds on CppInterOp we don't work on is using the old options they will break too.

This is not random. This is a requirement for ROOT and has been discussed with @vgvassilev
USE_CLING does not specify which library this options is for and is not good practice for Cmake, when integrating with projects with a larger build tree. This is what LLVM does too, as you can see all their options are prefixed with LLVM_

@aaronj0
Copy link
Collaborator Author

aaronj0 commented Feb 18, 2025

@mcbarton As for your concerns about the workflows, it's something that can be just updated. Other than the cppyy repos, xeus does not use this option anywhere since InterOp is not configured in any way and uses clang-repl by default.

@mcbarton
Copy link
Collaborator

@aaronj0 I don't understand the motivation for this change. Seems random. The PR description doesn't say why it needs to be done, just what is being done.
cc @vgvassilev
Also the workflows (and probably the code too even though I haven't checked) in cppyy and related repos all assume the old options, so will break a lot of things. If any other repo which builds on CppInterOp we don't work on is using the old options they will break too.

This is not random. This is a requirement for ROOT and has been discussed with @vgvassilev USE_CLING does not specify which library this options is for and is not good practice for Cmake, when integrating with projects with a larger build tree. This is what LLVM does too, as you can see all their options are prefixed with LLVM_

Even so, its best to have this reason in the PR description, because without it, anybody stumbling across the PR who was not party to the conversation between you and @vgvassilev would have no idea why this change was made. Can you make the required changes in the cppyy related repos, so they don't break with this change?

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. It's quite annoying that so many yaml files had to be updated.

@aaronj0 aaronj0 merged commit 6c6f94a into compiler-research:main Feb 18, 2025
74 checks passed
@vgvassilev
Copy link
Contributor

@aaronj0 I don't understand the motivation for this change. Seems random. The PR description doesn't say why it needs to be done, just what is being done.
cc @vgvassilev
Also the workflows (and probably the code too even though I haven't checked) in cppyy and related repos all assume the old options, so will break a lot of things. If any other repo which builds on CppInterOp we don't work on is using the old options they will break too.

This is not random. This is a requirement for ROOT and has been discussed with @vgvassilev USE_CLING does not specify which library this options is for and is not good practice for Cmake, when integrating with projects with a larger build tree. This is what LLVM does too, as you can see all their options are prefixed with LLVM_

Even so, its best to have this reason in the PR description, because without it, anybody stumbling across the PR who was not party to the conversation between you and @vgvassilev would have no idea why this change was made. Can you make the required changes in the cppyy related repos, so they don't break with this change?

That was an urgent request from our ROOT client and we need to act quickly...

aaronj0 added a commit to aaronj0/cppyy that referenced this pull request Feb 18, 2025
Based on the change in CppInterOp: compiler-research/CppInterOp#505 that prefixes the build option with `CPPINTEROP`
aaronj0 added a commit to aaronj0/cppyy-backend that referenced this pull request Feb 18, 2025
Based on the change in CppInterOp: compiler-research/CppInterOp#505 that prefixes the build option with `CPPINTEROP`
aaronj0 added a commit to aaronj0/CPyCppyy that referenced this pull request Feb 18, 2025
Based on the change in CppInterOp: compiler-research/CppInterOp#505 that prefixes the build option with `CPPINTEROP`
aaronj0 added a commit to compiler-research/cppyy that referenced this pull request Feb 18, 2025
Based on the change in CppInterOp: compiler-research/CppInterOp#505 that prefixes the build option with `CPPINTEROP`
aaronj0 added a commit to compiler-research/cppyy-backend that referenced this pull request Feb 18, 2025
Based on the change in CppInterOp: compiler-research/CppInterOp#505 that prefixes the build option with `CPPINTEROP`
aaronj0 added a commit to compiler-research/CPyCppyy that referenced this pull request Feb 18, 2025
Based on the change in CppInterOp: compiler-research/CppInterOp#505 that prefixes the build option with `CPPINTEROP`
aaronj0 added a commit to aaronj0/CPyCppyy that referenced this pull request Mar 7, 2025
Based on the change in CppInterOp: compiler-research/CppInterOp#505 that prefixes the build option with `CPPINTEROP`
aaronj0 added a commit to aaronj0/CPyCppyy that referenced this pull request Mar 18, 2025
Based on the change in CppInterOp: compiler-research/CppInterOp#505 that prefixes the build option with `CPPINTEROP`
@aaronj0 aaronj0 deleted the prefix-cmake-options branch April 22, 2025 08:28
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