Fix compile error when compiling boost graphs with recent clang #39526

saraedum · 2025-02-14T13:39:13Z

this fixes an underlying type confusion in our wrapper that has lead to warnings before but now produces an actual error, see #39249

this fixes an underlying type confusion in our wrapper that has lead to warnings before but now produces an actual error, see sagemath#39249

saraedum · 2025-02-14T13:39:49Z

src/sage/graphs/base/boost_interface.cpp

-public:
    adjacency_list graph;
    std::vector<vertex_descriptor> vertices;
    vertex_to_int_map index;

+public:


This is not necessary to fix the warning. But since these fields are not part of the Cython interface, they can as well be private (would have helped me to understand things more quickly.

github-actions · 2025-02-14T14:26:01Z

Documentation preview for this PR (built with commit 395ca33; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

user202729 · 2025-02-14T16:56:42Z

src/sage/graphs/base/boost_interface.cpp

-typedef int v_index;
-typedef long e_index;
+typedef unsigned long v_index;
+typedef unsigned long e_index;


I don't think this change is necessary. I think you got confused when the programmer lies to cython or something

https://cython-guidelines.readthedocs.io/en/latest/articles/lie.html

similar to #39098 (comment)

In other words: even though in the pxd the declaration is

ctypedef unsigned long v_index ctypedef unsigned long e_index

in compiled code it should still use int. (before the change)

Or so I think.

I don't understand why the programmer would want to lie to Cython here? So, e_index should be the type that num_edges returns. That's boost::graph_traits<G>::edges_size_type. I don't think I can reference that type directly from a pxd (but maybe there is a way, then I should probably use it.) Let me put a static assert into the code to check that this is actually unsigned long. Similarly, v_index is ~~vertex_index_t and~~ size_t and vertices_size_type. I guess they are all just unsigned long but I'll add a static assert to be sure.
This is not terribly clean. If we end up building on a platform where this is not true, then we might have to stop identifying these types but I think it's just the same type on all platforms anyway.

Maybe I'll put in the extra work and actually separate these types but I think in Cython I'll just set them all to unsigned long.

I am not sure I really understood your objection here though. Maybe you can rephrase what you had in mind?

I see now what you meant with the v_index. It's the type used in the definition of adjacency_list. So if I change it to an unsigned type, this actually changes something.

src/sage/graphs/base/boost_graph.pyx

user202729 · 2025-02-14T17:00:53Z

Anyway I think the problem is why is in the original code

    typedef typename boost::property_map<adjacency_list, boost::vertex_index_t>::type vertex_to_int_map;

the boost::vertex_index_t is not a v_index. And more importantly why is v_index different from boost::vertex_index_t? (1)

tl;dr while the two places changing int to v_index look good, I don't think the fix is semantically correct. Though I haven't got the time to fully understand the author's intention yet (see (1) above)

saraedum · 2025-02-14T20:34:22Z

Thanks for your thoughts, I'll try to make the code be a bit more careful with the types.

saraedum · 2025-02-14T21:42:05Z

Anyway I think the problem is why is in the original code
    typedef typename boost::property_map<adjacency_list, boost::vertex_index_t>::type vertex_to_int_map;
the boost::vertex_index_t is not a v_index. And more importantly why is v_index different from boost::vertex_index_t? (1)

I think boost::vertex_index_t is just used to single out a "slot" of the boost::property. It's not a type that is meant to be used for values I'd say.

user202729 · 2025-02-15T07:18:48Z

Okay, maybe I misunderstood.

But anyway, the "morally correct" typedef is to make v_index being type-def-ed to whichever type that

    typedef typename boost::property_map<adjacency_list, boost::vertex_index_t>::type vertex_to_int_map;
    vertex_to_int_map index;
index[boost::source(*ei, graph)]

belong to, right? (which is typename vertex_to_int_map::value_type, but I can't find where it's defined yet) Why is it unsigned long?

Or is that too much of a breaking API change and we should just cast it to v_index type instead?

Edit: because of

    typedef typename boost::adjacency_list<
        OutEdgeListS, VertexListS, DirectedS,
        boost::property<boost::vertex_index_t, v_index>,  # ⟵ this line
        EdgeProperty, boost::no_property, EdgeListS> adjacency_list;

then typename vertex_to_int_map::value_type is supposed to be v_index. But why is it not?

Edit: turns out it's because VertexListS is vecS or something else.

#include <boost/graph/adjacency_list.hpp>
#include <boost/property_map/property_map.hpp>
#include <boost/type_traits/is_same.hpp>
#include <type_traits>
#include <vector>

// type aliases remain unchanged
typedef int v_index;
typedef long e_index;

typedef boost::adjacency_list<
    boost::vecS,              // OutEdgeList
    boost::vecS,              // VertexList
    boost::undirectedS,       // DirectedS
    boost::property<boost::vertex_index_t, v_index>, // Vertex property.
    boost::no_property,       // Edge property (can be enhanced if needed).
    boost::no_property,       // Graph property.
    boost::vecS               // EdgeList
> Graph;

typedef boost::property_map<Graph, boost::vertex_index_t>::type vertex_to_int_map;

static_assert(std::is_same_v<typename vertex_to_int_map::value_type, v_index>,
              "The vertex property map's value type must be v_index");

when vecS is changed to listS then the static_assert passes.

Not sure if this is intended. I decide to open an issue on boost graph boostorg/graph#420 to see whether it is.

saraedum · 2025-02-15T13:18:08Z

I think it makes sense to split this into two separate issues. One that just fixes the compile error but keeps the status quo. And a second one that fixes all the confusion with the types that seems to be happening in the three .cpp .pxd and .pyx files that are involved here.
I don't think I have the bandwidth to work on the latter right now. But I'll create an issue to track this and try to push a fix for the first problem here.

tobiasdiez · 2025-02-28T14:31:28Z

At https://github.com/sagemath/sage/pull/38872/files#diff-5787682a0554d08f4dc34e60e792af0bbf764ecae08c8c941865e2573edf50f0, I have a more "minimal" fix. I'm lacking the background to see which one is "better".

            v_index source = index[boost::source(*ei, graph)];
            v_index target = index[boost::target(*ei, graph)];
            double weight = boost::get(boost::edge_weight, graph, *ei);
            to_return.push_back({source, {target, weight}});

user202729 · 2025-03-02T16:38:37Z

just fixes the compile error but keeps the status quo

I think we can just do this (which means performing an explicit casting at the use site, possibly adding a note explaining why the casting is needed)

(which is what the more minimal fix does.)

tobiasdiez · 2025-03-09T18:53:38Z

Setting this to CI Fix to resolve the macos build errors on macos.

dimpase · 2025-04-02T17:05:44Z

One way or another, it's a shame that 10.6 got released without any fix for this issue.

Does it also mean that a CI run testing sagelib build on macos conda is missing, or not required?

user202729 · 2025-04-02T23:05:53Z

Uh… "CI fix" means it's applied on CI, which has the downside of the hypothetical CI failure getting unnoticed.

I guess that slipped under everyone's radar.

dimpase

looks good to me

dimpase · 2025-04-03T19:48:12Z

please set the Positive review tag. I'm closing #39850 in favour of this one.

dimpase · 2025-04-03T19:52:39Z

This should be a blocker, and perhaps we should consider a 10.6.1 release including this one, as it breaks the build with Conda packages on macOS (such builds are documented as the easiest ones to get going...) @vbraun

user202729 · 2025-04-03T23:05:33Z

I think the problem I was waiting for was this

I think it makes sense to split this into two separate issues. One that just fixes the compile error but keeps the status quo. And a second one that fixes all the confusion with the types that seems to be happening in the three .cpp .pxd and .pyx files that are involved here. I don't think I have the bandwidth to work on the latter right now. But I'll create an issue to track this and try to push a fix for the first problem here.

Is the semantic unchanged in the current situation?

(Also you mean 10.6.1.)

dimpase · 2025-04-04T00:51:27Z

while this fix might be incomplete, it does the job, and it's a step in the right direction

user202729 · 2025-04-04T11:36:02Z

Okay (setting positive-review on dimpase's behalf, I'm not so sure on this)

sagemathgh-39526: Fix compile error when compiling boost graphs with recent clang this fixes an underlying type confusion in our wrapper that has lead to warnings before but now produces an actual error, see sagemath#39249 URL: sagemath#39526 Reported by: Julian Rüth Reviewer(s): Dima Pasechnik, Julian Rüth, user202729

sagemathgh-39526: Fix compile error when compiling boost graphs with recent clang this fixes an underlying type confusion in our wrapper that has lead to warnings before but now produces an actual error, see sagemath#39249 URL: sagemath#39526 Reported by: Julian Rüth Reviewer(s): Dima Pasechnik, Isuru Fernando, Julian Rüth, user202729

Fix compile error when compiling boost graphs with recent clang

395ca33

this fixes an underlying type confusion in our wrapper that has lead to warnings before but now produces an actual error, see sagemath#39249

github-actions bot added the s: needs review label Feb 14, 2025

saraedum commented Feb 14, 2025

View reviewed changes

user202729 reviewed Feb 14, 2025

View reviewed changes

src/sage/graphs/base/boost_graph.pyx Show resolved Hide resolved

user202729 mentioned this pull request Feb 15, 2025

Vertex property vertex_index_t silently discarded when vecS selector used vertex list boostorg/graph#420

Open

tobiasdiez mentioned this pull request Feb 28, 2025

Upgrade to pari 2.17.1, cypari 2.2.1, cysignals 1.12.3 #38749

Merged

tobiasdiez added the p: CI Fix merged before running CI tests label Mar 9, 2025

tobiasdiez mentioned this pull request Apr 2, 2025

correct the index type in Boost interface #39850

Closed

5 tasks

dimpase approved these changes Apr 3, 2025

View reviewed changes

user202729 added s: positive review and removed s: needs review labels Apr 4, 2025

isuruf approved these changes Apr 10, 2025

View reviewed changes

vbraun merged commit 8226bea into sagemath:develop Apr 18, 2025
29 of 30 checks passed

github-actions bot removed the s: positive review label Apr 18, 2025

Uh oh!

Fix compile error when compiling boost graphs with recent clang #39526

Fix compile error when compiling boost graphs with recent clang #39526

Uh oh!

Conversation

saraedum commented Feb 14, 2025

Uh oh!

saraedum Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 14, 2025

Uh oh!

user202729 Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

saraedum Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saraedum Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

user202729 commented Feb 14, 2025

Uh oh!

saraedum commented Feb 14, 2025

Uh oh!

saraedum commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

user202729 commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saraedum commented Feb 15, 2025

Uh oh!

tobiasdiez commented Feb 28, 2025

Uh oh!

user202729 commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez commented Mar 9, 2025

Uh oh!

dimpase commented Apr 2, 2025

Uh oh!

user202729 commented Apr 2, 2025

Uh oh!

dimpase left a comment

Choose a reason for hiding this comment

Uh oh!

dimpase commented Apr 3, 2025

Uh oh!

dimpase commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

user202729 commented Apr 3, 2025

Uh oh!

dimpase commented Apr 4, 2025

Uh oh!

user202729 commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

saraedum Feb 14, 2025 •

edited

Loading

saraedum commented Feb 14, 2025 •

edited

Loading

user202729 commented Feb 15, 2025 •

edited

Loading

user202729 commented Mar 2, 2025 •

edited

Loading

dimpase commented Apr 3, 2025 •

edited

Loading

user202729 commented Apr 4, 2025 •

edited

Loading