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

fix spatialite integration for windows #3580

Merged
merged 12 commits into from Mar 25, 2022
Merged

fix spatialite integration for windows #3580

merged 12 commits into from Mar 25, 2022

Conversation

nilsnolde
Copy link
Member

@nilsnolde nilsnolde commented Mar 18, 2022

fiiinalllyyy fixes #3010

been bugging me for ages..

I went through a few demos and chose a more "modern" approach to load spatialite. only tested it down to spatialite 4.3.0, which should be enough, even ubuntu 16.04 has it. it's also still a little rough around the edges maybe, let's see what you think.

I also (tried to) include a admin tile build in windows CI.

#include <sqlite3.h>
#include <unordered_map>

#include <spatialite.h>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to put those at the end as sqlite3.h needs to be included before spatialite.h. otherwise format.sh will simply switch the includes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I remove all references to sqlite/spatialite from mjolnir/util.h so we now get a lot more includes in cc files

Comment on lines 274 to 275
auto* db_cache = spatialite_alloc_connection();
spatialite_init_ex(db_handle, db_cache, 0);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is basically the new syntax. spatialite_init(0) is deprecated. we need to hold a pointer to the connection's address

@@ -99,7 +99,7 @@
"verbose": false
},
"mjolnir": {
"admin": "",
"admin": "test\\data\\netherlands_admin.sqlite",
Copy link
Member Author

@nilsnolde nilsnolde Mar 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trying it out on azure (worked locally ;))

@nilsnolde
Copy link
Member Author

nilsnolde commented Mar 18, 2022

Huh strange.. of all things, the Python bindings build triggers a leak in build_tile_set()..

wasn't the bindings of course.. issue was calling spatialite_shutdown() prematurely and multiple times.

@irees
Copy link
Collaborator

irees commented Mar 18, 2022

I'm also having an issue, possibly unrelated, with spatialite header file locations being wrong (even when I manually specify it) on macOS/arm. Maybe I'll have a fix for that soon. 😄

@kevinkreiser
Copy link
Member

i want to make a few suggestions to this. mainly i want to encapsulate the connection init and cleanup in a shared ptr and i want to cencapsulate the global init and shutdown in a singleton. i also want to join this up with the sqlite stuff that it interacts with. i think all this crap can live in a single header with the sqlite stuff. ill make a suggestion later on tonight!

the start of it will be something like this

// we call initialize once the first time its accessed and we shutdown once when it goes out of scope (end of program)
struct spatialite_singleton_t {
  static const spatialite_singleton_t& get_instance() {
    static spatialite_singleton_t s;
    return s;
  }
private:
  spatialite_singleton_t() {
    spatialite_initialize();
  }
  ~spatialite_singleton_t() {
    spatialite_shutdown();
  }
};


// wrap the connection in a shared pointer so its automatically cleaned up when no longer used
std::shared_ptr<void> make_spatialite_cache(sqlite3* handle) {
  const auto& s = spatialite_singleton_t::get_instance();
  void* conn = spatialite_alloc_connection();
  spatialite_init_ex(handle, conn, 0);
  return  std::shared_ptr<void>(conn, [](void* c){spatialite_cleanup_ex(c);});
}

@nilsnolde
Copy link
Member Author

I took a stab at implementing your suggestion, thanks for that! much cleaner.

I thought I'd include it all in a private header in ./src/mjolnir/spatialite_conn.h, but the linker complained that there are multiple definitions in graphbuilder.cc and graphenhancer.cc, though I didn't see what might be causing that.

@kevinkreiser
Copy link
Member

@nilsnolde you need to make the functions inline or make a cc file and put the guts of the implementation in there with just the definitions in the header

@nilsnolde
Copy link
Member Author

ah inlining would've helped there? good to know.. but yeah, I went for your second suggestion, .h/.cc file.

@@ -75,6 +76,7 @@
* ADDED: Expose reverse isochrone parameter for reverse expansion [#3528](https://github.com/valhalla/valhalla/pull/3528)
* CHANGED: Add matrix classes to thor worker so they persist between requests. [#3560](https://github.com/valhalla/valhalla/pull/3560)
* CHANGED: Remove `max_matrix_locations` and introduce `max_matrix_location_pairs` to configure the allowed number of total routes for the matrix action for more flexible asymmetric matrices [#3569](https://github.com/valhalla/valhalla/pull/3569)
* CHANGED: modernized spatialite syntax [#3580](https://github.com/valhalla/valhalla/pull/3580)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somehow felt like 2 entries, let me know if I should remove one

@nilsnolde nilsnolde merged commit 6db08de into master Mar 25, 2022
@nilsnolde nilsnolde deleted the nn-spatialite-fix branch March 25, 2022 15:34
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.

Fix spatialite support for Windows
3 participants