search

osm-search / Nominatim

Open Source search based on OpenStreetMap data
https://nominatim.org
GNU General Public License v3.0
3.05k stars 711 forks source link

test suite assumes osm2psql located in build path #3469

Closed mtmail closed 3 weeks ago

mtmail commented 1 month ago

https://github.com/osm-search/Nominatim/pull/3458 notes "osm2pgsql will no longer be bundled with Nominatim when installing with pip."

The test suite still looks only in the build directory for osm2psql.

grep -r osm2pgsql test | grep build
test/bdd/steps/nominatim_environment.py:        self.test_env['NOMINATIM_OSM2PGSQL_BINARY'] = str((self.build_dir / 'osm2pgsql' / 'osm2pgsql').resolve())
test/bdd/steps/nominatim_environment.py:                        osm2pgsql=self.build_dir / 'osm2pgsql' / 'osm2pgsql')
test/bdd/steps/nominatim_environment.py:                      osm2pgsql_path=str(self.build_dir / 'osm2pgsql' / 'osm2pgsql'),
test/bdd/steps/steps_osm_data.py:                osm2pgsql=str(nominatim_env.build_dir / 'osm2pgsql' / 'osm2pgsql'),

Should it look for system-wide installed osm2psql if the build_dir doesn't exist? Or should the documentation docs/develop/Development-Environment.md be updated instead? (I just created a symlink and that worked)

lonvia commented 1 month ago

I would suggest that we always rely on an external osm2pgsql for the tests. It will simplify the code a bit.

I'm in the middle of porting the code to psycopg3 which has quite some code changes around the BDD setup code. So I would prefer to defer this issue until after the psycopg upgrade is through.

mtmail commented 1 month ago

Ok, so I delay the PR. The symlink works just fine in the meantime.

lonvia commented 3 weeks ago

So for the time being we still need the build directory when testing against the legacy tokenizer. I'll do a PR where the default for the build directory is None but you can still set it directly.

The other solution would be to just drop the legacy tokenizer with the next version. I'm seriously tempted to go down that road because it already is somewhat broken and the CI didn't properly catch it.