Install missing dependencies MacOS cppyy jobs #125

mcbarton · 2025-01-23T10:34:33Z

There are a lot of tests failing for things simply not being installed on MacOS. Hopefully by installing these cppyy will get more passing tests on MacOS. See here about missing dependencies https://github.com/compiler-research/CppInterOp/actions/runs/12925582648/job/36047197449#step:14:7103

test/test_lowlevel.py

Vipul-Cariappa

Thank you for this. The changes look good to me.

If you can make the change I have specified, it would be helpful. skip should only be used when we are missing libraries/dependencies, otherwise it should always be xfail.

test/test_lowlevel.py

aaronj0 · 2025-01-28T04:57:37Z

test/test_boost.py

@@ -7,15 +7,12 @@
        os.path.exists(os.path.join(os.path.sep, 'usr', 'local', 'include', 'boost'))):
    noboost = True

-
-@mark.skipif(noboost == True, reason="boost not found")


This skip tag is default on upstream cppyy should not be removed as it will cause the tests to run on platforms that do not have libboost installed

Ok. Then we need a better test to check if boost is on the system. Its check to just see if /usr/local/include/boost exists is too basic and only applies to Linux. MacOS doesn't put them there for both homebrew and Macports.

How about we do something like this:

try: cppyy.include('boost/any.hpp') noboost = True except ImportError: noboost = False

aaronj0 · 2025-01-28T04:58:59Z

test/test_boost.py

@@ -67,8 +64,6 @@ def test02_any_usage(self):
        extract = boost.any_cast[std.vector[int]](val)
        assert len(extract) == 200

-
-@mark.skipif(((noboost == True) or IS_MAC_ARM or IS_MAC_X86), reason="boost not found")


Here we can remove the or IS_MAC_ARM or IS_MAC_X86 part, and retain the tag as mentioned in the above comment

aaronj0 · 2025-01-28T04:59:07Z

test/test_boost.py

@@ -92,8 +87,6 @@ class Derived : boost::ordered_field_operators<Derived>, boost::ordered_field_op

        assert cppyy.gbl.boost_test.Derived

-
-@mark.skipif(noboost == True, reason="boost not found")


Same as above comment

aaronj0 · 2025-01-28T04:59:24Z

test/test_eigen.py

@@ -11,8 +11,6 @@
    if os.path.exists(p):
        eigen_path = p

-
-@mark.skipif(eigen_path is None, reason="Eigen not found")


Same for this tag as well

aaronj0 · 2025-01-28T05:00:00Z

test/test_boost.py

@@ -135,7 +128,6 @@ class C { }; } """)
        assert type(boost.get['BV::C'](v[2])) == cpp.BV.C


-@mark.skipif(((noboost == True) or IS_MAC_ARM or IS_MAC_X86), reason="boost not found")


Here we can remove the or IS_MAC_ARM or IS_MAC_X86, and retain the tag as mentioned in the above comment

aaronj0 · 2025-01-28T05:00:20Z

test/test_eigen.py

@@ -148,7 +146,6 @@ def test04_resizing_through_assignment(self):
        assert a.size() == 9


-@mark.skipif(eigen_path is None, reason="Eigen not found")


Same for this tag as well

Vipul-Cariappa · 2025-01-30T06:30:45Z

Hi @aaronj0, can we do this

diff --git a/test/test_boost.py b/test/test_boost.py
index a000de3..3204993 100644
--- a/test/test_boost.py
+++ b/test/test_boost.py
@@ -1,11 +1,15 @@
 import py, os, sys
+import cppyy
 from pytest import mark, raises, skip
 from .support import setup_make, IS_CLANG_REPL, IS_MAC_X86, IS_MAC_ARM
 
-noboost = False
-if not (os.path.exists(os.path.join(os.path.sep, 'usr', 'include', 'boost')) or \
-        os.path.exists(os.path.join(os.path.sep, 'usr', 'local', 'include', 'boost'))):
-    noboost = True
+noboost = not cppyy.gbl.Cpp.Evaluate("""
+    #if __has_include("boost/any.hpp")
+        true
+    #else
+        false
+    #endif
+""")
 
 
 @mark.skipif(noboost == True, reason="boost not found")
diff --git a/test/test_eigen.py b/test/test_eigen.py
index 187e244..fcce273 100644
--- a/test/test_eigen.py
+++ b/test/test_eigen.py
@@ -1,26 +1,28 @@
 import py, os, sys
+import cppyy
 from pytest import mark, raises
 from .support import setup_make, IS_CLANG_REPL, IS_MAC_X86
 
 inc_paths = [os.path.join(os.path.sep, 'usr', 'include'),
              os.path.join(os.path.sep, 'usr', 'local', 'include')]
 
-eigen_path = None
-for p in inc_paths:
-    p = os.path.join(p, 'eigen3')
-    if os.path.exists(p):
-        eigen_path = p
+noeigen = not cppyy.gbl.Cpp.Evaluate("""
+    #if __has_include("eigen3/Eigen/Dense")
+        true
+    #else
+        false
+    #endif
+""")
 
 
-@mark.skipif(eigen_path is None, reason="Eigen not found")
+@mark.skipif(noeigen == True, reason="Eigen not found")
 class TestEIGEN:
     def setup_class(cls):
         import cppyy, warnings
 
-        cppyy.add_include_path(eigen_path)
         with warnings.catch_warnings():
             warnings.simplefilter('ignore')
-            cppyy.include('Eigen/Dense')
+            cppyy.include('eigen3/Eigen/Dense')
 
     @mark.xfail
     def test01_simple_matrix_and_vector(self):
@@ -148,15 +150,14 @@ class TestEIGEN:
         assert a.size() == 9
 
 
-@mark.skipif(eigen_path is None, reason="Eigen not found")
+@mark.skipif(noeigen == True, reason="Eigen not found")
 class TestEIGEN_REGRESSIOn:
     def setup_class(cls):
         import cppyy, warnings
 
-        cppyy.add_include_path(eigen_path)
         with warnings.catch_warnings():
             warnings.simplefilter('ignore')
-            cppyy.include('Eigen/Dense')
+            cppyy.include('eigen3/Eigen/Dense')
 
     @mark.xfail(condition=IS_MAC_X86 and (not IS_CLANG_REPL), reason="Errors out on OS X Cling")
     def test01_use_of_Map(self):

?
This feels safe and valid to me.

aaronj0 · 2025-01-30T08:57:15Z

Hi @aaronj0, can we do this

diff --git a/test/test_boost.py b/test/test_boost.py
index a000de3..3204993 100644
--- a/test/test_boost.py
+++ b/test/test_boost.py
@@ -1,11 +1,15 @@
 import py, os, sys
+import cppyy
 from pytest import mark, raises, skip
 from .support import setup_make, IS_CLANG_REPL, IS_MAC_X86, IS_MAC_ARM
 
-noboost = False
-if not (os.path.exists(os.path.join(os.path.sep, 'usr', 'include', 'boost')) or \
-        os.path.exists(os.path.join(os.path.sep, 'usr', 'local', 'include', 'boost'))):
-    noboost = True
+noboost = not cppyy.gbl.Cpp.Evaluate("""
+    #if __has_include("boost/any.hpp")
+        true
+    #else
+        false
+    #endif
+""")
 
 
 @mark.skipif(noboost == True, reason="boost not found")
diff --git a/test/test_eigen.py b/test/test_eigen.py
index 187e244..fcce273 100644
--- a/test/test_eigen.py
+++ b/test/test_eigen.py
@@ -1,26 +1,28 @@
 import py, os, sys
+import cppyy
 from pytest import mark, raises
 from .support import setup_make, IS_CLANG_REPL, IS_MAC_X86
 
 inc_paths = [os.path.join(os.path.sep, 'usr', 'include'),
              os.path.join(os.path.sep, 'usr', 'local', 'include')]
 
-eigen_path = None
-for p in inc_paths:
-    p = os.path.join(p, 'eigen3')
-    if os.path.exists(p):
-        eigen_path = p
+noeigen = not cppyy.gbl.Cpp.Evaluate("""
+    #if __has_include("eigen3/Eigen/Dense")
+        true
+    #else
+        false
+    #endif
+""")
 
 
-@mark.skipif(eigen_path is None, reason="Eigen not found")
+@mark.skipif(noeigen == True, reason="Eigen not found")
 class TestEIGEN:
     def setup_class(cls):
         import cppyy, warnings
 
-        cppyy.add_include_path(eigen_path)
         with warnings.catch_warnings():
             warnings.simplefilter('ignore')
-            cppyy.include('Eigen/Dense')
+            cppyy.include('eigen3/Eigen/Dense')
 
     @mark.xfail
     def test01_simple_matrix_and_vector(self):
@@ -148,15 +150,14 @@ class TestEIGEN:
         assert a.size() == 9
 
 
-@mark.skipif(eigen_path is None, reason="Eigen not found")
+@mark.skipif(noeigen == True, reason="Eigen not found")
 class TestEIGEN_REGRESSIOn:
     def setup_class(cls):
         import cppyy, warnings
 
-        cppyy.add_include_path(eigen_path)
         with warnings.catch_warnings():
             warnings.simplefilter('ignore')
-            cppyy.include('Eigen/Dense')
+            cppyy.include('eigen3/Eigen/Dense')
 
     @mark.xfail(condition=IS_MAC_X86 and (not IS_CLANG_REPL), reason="Errors out on OS X Cling")
     def test01_use_of_Map(self):

? This feels safe and valid to me.

Yes, this seems valid. I propose we should verify the same on platforms without eigen and boost, and an easy way would be to drop their installations from the workflow on this PR temporarily(and add back the tests that were completely dropped)

@mcbarton Can you try to apply these suggestions?

Co-authored-by: Vipul Cariappa <[email protected]>

mcbarton · 2025-02-02T17:52:08Z

@aaronj0 I've discussed this PR with @Vipul-Cariappa , and I said it would be ok for him to take this PR forward, and apply the suggestions he made.

Vipul-Cariappa

Hi @mcbarton. These changes look good. I have updated it with the changes I had mentioned previously in one of my comments.

Looking at the logs, OSX with clang-repl is not able to find eigen.

But OSX with cling is able to find eigen.

No issues with boost.

Can you look into this. Otherwise this is ready to be merged.

mcbarton · 2025-02-03T11:20:11Z

Its probably that Eigen hasn't been added the CPLUS_INCLUDE_PATH for it to be found. Will look into this later today.

mcbarton · 2025-02-04T11:47:36Z

@Vipul-Cariappa @aaronj0 My fix to get cppyy to find Eigen (adding it to CPLUS_INCLUDE_PATH) appears to work. If you can double check I think this PR is ready for approval and merging.

Vipul-Cariappa

LGTM!

I have cross checked. No regressions anywhere. The logs are as I would want it to be. +4 tests pass in OSX.

This reverts commit bf43358.

aaronj0 · 2025-02-06T14:36:22Z

LGTM!

I have cross checked. No regressions anywhere. The logs are as I would want it to be. +4 tests pass in OSX.

@Vipul-Cariappa Did you also do this:

I propose we should verify the same on platforms without eigen and boost, and an easy way would be to drop their installations from the workflow on this PR temporarily

We need to make sure that this doesn't error out if boost isn't found, like it does now that causes the CI to fail...

This reverts commit bf43358.

Vipul-Cariappa · 2025-02-06T14:45:15Z

@aaronj0

Yes. I did it. Between commits "update checking external package for testing" and "Try to fix issue of Eigen not found".

But I guess I skipped on testing boost. I made sure that eigen did not cause that problem. Assumed boost will not cause any problem as the logic for both are the same.

aaronj0 · 2025-02-06T14:53:57Z

@aaronj0

Yes. I did it. Between commits "update checking external package for testing" and "Try to fix issue of Eigen not found".

But I guess I skipped on testing boost. I made sure that eigen did not cause that problem. Assumed boost will not cause any problem as the logic for both are the same.

That is not what I meant. Your check may work since the CI pip/uv/brew installs the packages but we should drop their installations on the CI and ensure the tests don't error out.

Vipul-Cariappa · 2025-02-06T14:56:15Z

Yes. We should do it. As the CI is failing.
Will do it tomorrow.

mcbarton force-pushed the Install-needed-deps-for-MacOS-cppyy-tests branch from a402ced to ddc7891 Compare January 23, 2025 16:26

vgvassilev reviewed Jan 23, 2025

View reviewed changes

test/test_lowlevel.py Outdated Show resolved Hide resolved

Vipul-Cariappa approved these changes Jan 24, 2025

View reviewed changes

test/test_lowlevel.py Outdated Show resolved Hide resolved

mcbarton requested review from Vipul-Cariappa and aaronj0 January 27, 2025 15:53

aaronj0 requested changes Jan 28, 2025

View reviewed changes

mcbarton and others added 5 commits January 30, 2025 18:58

Install missing dependencies MacOS cppyy jobs

0fb3887

label failing leak check tests on macOS as xfail

3e7e22e

Update test/test_lowlevel.py

f99f339

Co-authored-by: Vipul Cariappa <[email protected]>

Update test_lowlevel.py

c7e3713

update checking external package for testing

3c1a374

Vipul-Cariappa force-pushed the Install-needed-deps-for-MacOS-cppyy-tests branch from 85473ba to 3c1a374 Compare January 30, 2025 13:33

Vipul-Cariappa approved these changes Feb 3, 2025

View reviewed changes

Try to fix issue of Eigen not found

669947a

mcbarton requested review from aaronj0 and Vipul-Cariappa February 4, 2025 11:47

Vipul-Cariappa approved these changes Feb 4, 2025

View reviewed changes

Vipul-Cariappa merged commit bf43358 into compiler-research:master Feb 4, 2025
58 checks passed

mcbarton mentioned this pull request Feb 4, 2025

Install missing dependencies osx jobs to fix osx ci workflows compiler-research/CppInterOp#496

Closed

4 tasks

Vipul-Cariappa added a commit that referenced this pull request Feb 6, 2025

Revert "Install missing dependencies MacOS cppyy jobs (#125)"

d19b78e

This reverts commit bf43358.

Vipul-Cariappa mentioned this pull request Feb 6, 2025

Revert "Install missing dependencies MacOS cppyy jobs" #133

Merged

aaronj0 pushed a commit that referenced this pull request Feb 6, 2025

Revert "Install missing dependencies MacOS cppyy jobs (#125)"

c80f30d

This reverts commit bf43358.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install missing dependencies MacOS cppyy jobs #125

Install missing dependencies MacOS cppyy jobs #125

mcbarton commented Jan 23, 2025 •

edited

Loading

Vipul-Cariappa left a comment

aaronj0 Jan 28, 2025

mcbarton Jan 28, 2025

Vipul-Cariappa Jan 28, 2025

aaronj0 Jan 28, 2025

aaronj0 Jan 28, 2025

aaronj0 Jan 28, 2025

aaronj0 Jan 28, 2025

aaronj0 Jan 28, 2025

Vipul-Cariappa commented Jan 30, 2025

aaronj0 commented Jan 30, 2025 •

edited

Loading

mcbarton commented Feb 2, 2025

Vipul-Cariappa left a comment

mcbarton commented Feb 3, 2025

mcbarton commented Feb 4, 2025

Vipul-Cariappa left a comment

aaronj0 commented Feb 6, 2025 •

edited

Loading

Vipul-Cariappa commented Feb 6, 2025

aaronj0 commented Feb 6, 2025 •

edited

Loading

Vipul-Cariappa commented Feb 6, 2025

		@@ -92,8 +87,6 @@ class Derived : boost::ordered_field_operators<Derived>, boost::ordered_field_op

		assert cppyy.gbl.boost_test.Derived


		@mark.skipif(noboost == True, reason="boost not found")

		@@ -135,7 +128,6 @@ class C { }; } """)
		assert type(boost.get['BV::C'](v[2])) == cpp.BV.C


		@mark.skipif(((noboost == True) or IS_MAC_ARM or IS_MAC_X86), reason="boost not found")

		@@ -148,7 +146,6 @@ def test04_resizing_through_assignment(self):
		assert a.size() == 9


		@mark.skipif(eigen_path is None, reason="Eigen not found")

Install missing dependencies MacOS cppyy jobs #125

Install missing dependencies MacOS cppyy jobs #125

Conversation

mcbarton commented Jan 23, 2025 • edited Loading

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipul-Cariappa commented Jan 30, 2025

aaronj0 commented Jan 30, 2025 • edited Loading

mcbarton commented Feb 2, 2025

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

mcbarton commented Feb 3, 2025

mcbarton commented Feb 4, 2025

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

aaronj0 commented Feb 6, 2025 • edited Loading

Vipul-Cariappa commented Feb 6, 2025

aaronj0 commented Feb 6, 2025 • edited Loading

Vipul-Cariappa commented Feb 6, 2025

mcbarton commented Jan 23, 2025 •

edited

Loading

aaronj0 commented Jan 30, 2025 •

edited

Loading

aaronj0 commented Feb 6, 2025 •

edited

Loading

aaronj0 commented Feb 6, 2025 •

edited

Loading