search

Project-OSRM / osrm-backend

Open Source Routing Machine - C++ backend
http://map.project-osrm.org
BSD 2-Clause "Simplified" License
6.29k stars 3.32k forks source link

[Build Issue] Building OSRM with cmake on MacOS fails - Preprocessor Guard Suggested #6589

Closed AlTimofeyev closed 1 year ago

AlTimofeyev commented 1 year ago

Issue

I was trying to build OSRM project for GSoC following the Wiki Instructions on MacOS Version 13.2.1. The build fails with error every time I try to build:

error: variable 'block_counter' set but not used [-Werror,-Wunused-but-set-variable]
        unsigned block_counter = 0;

Possible Solution

From doing a quick search, I think commenting out or removing the unused variable from the following location, osrm-backend/include/util/range_table.hpp:82:18, might fix this issue? Following the call-back history:

Better possible solution proposed in comment directly below this post.

Steps to reproduce

Specifications

Development Environment.

Similar Issue

I found a couple similar issues related to building OSRM on MacOS:

AlTimofeyev commented 1 year ago

I take back my previous suggestion for a possible solution, removing/commenting-out the unused variable block_counter is not good as it's actually used in four other locations in osrm-backend/include/util/range_table.hpp:

Looking at this stackoverflow post, warning: variable set but not used [-Wunused-but-set-variable], someone suggests to use preprocessor guards around the declaration and initialization of the variable.

From looking at the code in osrm-backend/include/util/range_table.hpp:

// construct table from length vector
template <typename VectorT> explicit RangeTable(const VectorT &lengths)
{
   // [... other code ... ]

   unsigned block_counter = 0;

   // [... other code ... ]

   for (const unsigned l : lengths)
   {
      // [... other code ... ]
      // block_counter is only used in this loop.
   }

// [... other code ... ]
}

The use of the unused variable block_counter is dependent on if the Table has a length > 0.

New Proposed Solution

Surround the for loop, and the variables used in the for loop, with preprocessor guards:

// construct table from length vector
template <typename VectorT> explicit RangeTable(const VectorT &lengths)
{
   // [... other code ... ]

#if lengths.size() > 0
   unsigned block_counter = 0;

   for (const unsigned l : lengths)
   {
      // [... other code ... ]
      // block_counter is only used in this loop.
   }
#endif

// [... other code ... ]
}
SiarheiFedartsou commented 1 year ago

I would do the same as it is already done with block_sum variable in the same place https://github.com/Project-OSRM/osrm-backend/blob/d51631401eeab6e90fa58b29f2ba8c0f56dc1161/include/util/range_table.hpp#L85 (see BOOST_ASSERT_IS_VOID)

it is a bit weird that we had this warning for block_sum in the past, but not for block_counter 🤔

AlTimofeyev commented 1 year ago

Okay, now I understand why I kept seeing BOOST_ASSERT_IS_VOID in that section of the code. I was wondering what it was used for. Thank you for that!

I have updated the osrm-backend/include/util/range_table.hpp file locally from:

// construct table from length vector
template <typename VectorT> explicit RangeTable(const VectorT &lengths)
{
   // [... other code ... ]
   unsigned block_counter = 0;

#ifndef BOOST_ASSERT_IS_VOID
        unsigned block_sum = 0;
#endif

   for (const unsigned l : lengths)
   {
      // [... other code ... ]

      // block is full
      if (BLOCK_SIZE == block_idx)
      {
         diff_blocks.push_back(block);
         block_counter++;
      }
   }

// [... other code ... ]
}

To:

// construct table from length vector
template <typename VectorT> explicit RangeTable(const VectorT &lengths)
{
   // [... other code ... ]

#ifndef BOOST_ASSERT_IS_VOID
        unsigned block_sum = 0;
        unsigned block_counter = 0;
#endif

   for (const unsigned l : lengths)
   {
      // [... other code ... ]

      // block is full
      if (BLOCK_SIZE == block_idx)
      {
         diff_blocks.push_back(block);
#ifndef BOOST_ASSERT_IS_VOID
         block_counter++;
#endif
      }
   }

// [... other code ... ]
}

This solved the issue of the unused variable! The build process did fail again though, during the cmake --build . phase when it was linking an executable:

[... everything successful above ...]
[ 82%] Building CXX object CMakeFiles/osrm-routed.dir/src/tools/routed.cpp.o
[ 82%] Linking CXX executable osrm-routed

[...]

ld: 30 duplicate symbols for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [osrm-routed] Error 1
make[1]: *** [CMakeFiles/osrm-routed.dir/all] Error 2
make: *** [all] Error 2

However, I followed the solution you offered for this type of error in issue #6566 and I downgraded my version of boost to version 1.76 via Homebrew and re-ran the build step and everything was able to complete just fine. Thank you again for your help!!

Would you like me to open up a PR with that small change to the osrm-backend/include/util/range_table.hpp file I mentioned? I was planning on try and becoming a contributor to this project (via GSoC, but that's too late now) but if you'd prefer otherwise, I completely understand and please let me know.

SiarheiFedartsou commented 1 year ago

Would you like me to open up a PR with that small change to the osrm-backend/include/util/range_table.hpp file I mentioned?

Sure, any contributions are very welcome :+1:

I was planning on try and becoming a contributor to this project (via GSoC, but that's too late now) but if you'd prefer otherwise, I completely understand and please let me know.

Yeah, unfortunately it is too late to apply for GSoC, but if you want to contribute into this project - you are welcome, I will try to help with anything needed.

AlTimofeyev commented 1 year ago

Fantastic, Thank you! I'll fork over the project and open a PR after I make the tiny change tomorrow.