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

Set compiler for OSRM build #1101

Closed
wants to merge 4 commits into from

Conversation

kkarbowiak
Copy link
Contributor

Issue #1098

Tasks

  • Update CHANGELOG.md (remove if irrelevant)
  • review

@kkarbowiak
Copy link
Contributor Author

While current master branch compiles fine using clang-15 on my machine, the v5.27.1 release sadly does not. I guess I forgot about the compilation problems with this project.

@kkarbowiak
Copy link
Contributor Author

Compilation with g++-12 fails for both v5.27.1 and current master.

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 25, 2024

Building OSRM with g++-12 works on v5.27.1 when adding -DCMAKE_CXX_FLAGS="-Wno-array-bounds -Wno-uninitialized" to the first cmake .. command. There are a couple issues related to the error without those flags on the OSRM bugtracker. Long story short this is how it's done on the OSRM CI pipeline for gcc-12.

I'll try to investigate the clang-15 errors since we'll have to sort them out in some way if we ever want #1080 to happen before another upstream release.

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 25, 2024

The clang++-15 errors are fairly simple and have been fixed upstream in master in various commits. Porting the changes back to v5.27.1 boils down to applying this patch:

diff --git a/include/util/range_table.hpp b/include/util/range_table.hpp
index aa1b89f1c..31379ada6 100644
--- a/include/util/range_table.hpp
+++ b/include/util/range_table.hpp
@@ -81,10 +81,10 @@ template <unsigned BLOCK_SIZE, storage::Ownership Ownership> class RangeTable
         unsigned last_length = 0;
         unsigned lengths_prefix_sum = 0;
         unsigned block_idx = 0;
-        unsigned block_counter = 0;
         BlockT block;
 #ifndef BOOST_ASSERT_IS_VOID
         unsigned block_sum = 0;
+        unsigned block_counter = 0;
 #endif
         for (const unsigned l : lengths)
         {
@@ -111,7 +111,9 @@ template <unsigned BLOCK_SIZE, storage::Ownership Ownership> class RangeTable
             if (BLOCK_SIZE == block_idx)
             {
                 diff_blocks.push_back(block);
+#ifndef BOOST_ASSERT_IS_VOID
                 block_counter++;
+#endif
             }
 
             // we can only store strings with length 255
diff --git a/src/contractor/graph_contractor.cpp b/src/contractor/graph_contractor.cpp
index 0b3e87cc2..17a386f11 100644
--- a/src/contractor/graph_contractor.cpp
+++ b/src/contractor/graph_contractor.cpp
@@ -645,7 +645,6 @@ std::vector<bool> contractGraph(ContractorGraph &graph,
 
     const util::XORFastHash<> hash;
 
-    unsigned current_level = 0;
     std::size_t next_renumbering = number_of_nodes * 0.35;
     while (remaining_nodes.size() > number_of_core_nodes)
     {
@@ -761,7 +760,6 @@ std::vector<bool> contractGraph(ContractorGraph &graph,
         remaining_nodes.resize(begin_independent_nodes_idx);
 
         p.PrintStatus(number_of_contracted_nodes);
-        ++current_level;
     }
 
     node_data.Renumber(new_to_old_node_id);

Unfortunately the two commits containing those changes also contain other changes in other files (or the same files) so that it is no possible to simply cherry-pick just the above on top of v5.27.1.

What about storing this patch somewhere in our include folder and applying in CI? I'm aware this would only be a temporary workaround, but as far as OSRM releases are concerned a temporary workaround may be here to stay...

@jcoupey jcoupey added the CI label Apr 25, 2024
@jcoupey jcoupey added this to the v1.15.0 milestone Apr 25, 2024
@jcoupey
Copy link
Collaborator

jcoupey commented May 22, 2024

Closing here as the scope of this PR has been addressed as part of #1113. Thanks @kkarbowiak for the include tip for additional variables in the worfklow.

@jcoupey jcoupey closed this May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants