nik4: init at 1.8 #366194
nik4: init at 1.8 #366194
Conversation
github-actions
bot
added
6.topic: python
6.topic: nixos
|
Also see https://nixcademy.com/posts/nixos-openstreetmap/ and https://github.com/tfc/nixos-openstreetmap, which does something similar. |
|
I plan to create similar NixOS tests for calculating a route between two coordinates and also for full text search in the future. |
ofborg
bot
added
8.has: package (new)
|
Great work @Luflosi ! I'll review ASAP. |
There was a problem hiding this comment.
@Luflosi , thank you very much for interesting work and sorry for waiting so long for review.
I suggested some changes and might have a few comments later, but I'm pretty sure we get this merged soon !
BTW, you are very welcome to join Nix Geospatial Team.
| @@ -0,0 +1,210 @@ | |||
| { | |||
| carto, | |||
There was a problem hiding this comment.
Personally, I like when the list of function parameters is logically separated - lib and functions first and then packages (see gdal as an example).
|
|
||
| # The original get-fonts.sh is currently broken, see https://github.com/gravitystorm/openstreetmap-carto/issues/5013 | ||
| # This original script also does not aim to reproducible. It does not do any commit pinning or checksumming. | ||
| get_fonts = writeShellApplication { |
There was a problem hiding this comment.
What is the purpose of get_fonts script ? Also, I would try to avoid to run nix CLI (nix-store) here.
There was a problem hiding this comment.
The original get_fonts.sh script downloads fonts needed for properly rendering the map into a new directory in the current working directory. You can see an example of it being used (or at least prepared for use) here: https://github.com/tfc/nixos-openstreetmap/blob/0fd30b016eb838395d85948b9ecf00ff59b4581d/openstreetmap.nix#L67-L74. In that example renderd later makes use of these files.
I don't like the original script:
- It's currently broken, see
get-fontsfails, due to fonts.google.com not returning a zip. gravitystorm/openstreetmap-carto#5013 - It spews warnings while running
- It downloads old versions of the fonts from (among other places) a now archived repository
- It doesn't do any checksumming or signature verification
So I replaced it with my own version which just reuses fonts already packaged in Nixpkgs. I'm not using it in the NixOS test because I can pass the fonts directly to Nik4, which is more declarative than running an imperative step first.
The reason why I chose to use the Nix CLI instead of copying the files was that I disliked the files being duplicated on disk. Nix just creates a symlink. But since you seem to really dislike using the Nix CLI, I replaced the call tonixwithcp. I had to use--dereferencesince otherwisecpjust copies the symlinks to the files in the Nix store themselves, which are not known to the garbage collector and will thus be deleted eventually.
| "out" | ||
| "sql" | ||
| "lua" | ||
| "get_external_data" |
There was a problem hiding this comment.
I suggest to rename this output.
| "get_external_data" | |
| "external_data" |
There was a problem hiding this comment.
The script is called get-external-data.py, so I named the output get_external_data. The output does not directly contain the external data but only a script that knows how to "get" it. Do you still think that I should rename it?
There was a problem hiding this comment.
I am still in favor calling it external_data.
| inherit hash; | ||
| }; | ||
| }; | ||
| simplified_water_polygons = mkArchive { |
There was a problem hiding this comment.
What about moving all data downloading code to nixos/tests/nik4.nix test and generate external-data.yml file required for test script there ? It looks to me as a more appropriate place.
There was a problem hiding this comment.
My goal was to make this script as reproducible as possible, no matter if it's called in the NixOS test or on a users system. I'm in favor of keeping it this way.
There was a problem hiding this comment.
What is the purpose of this data ? Isn't it useful mostly for testing ? If that's true, I still lean more towards moving this code to the test.
Another issue of having this code in package is how to update this data over the time (and how often). We might end up with reproducible but very outdated data.
What do you think @autra ?
|
|
||
| # The original get-fonts.sh is currently broken, see https://github.com/gravitystorm/openstreetmap-carto/issues/5013 | ||
| # This original script also does not aim to be reproducible. It does not do any commit pinning or checksumming. | ||
| get-fonts = writeShellApplication { |
There was a problem hiding this comment.
We shouldn't need to copy them. If I understand correctly, create a store path with all the needed font with symlinkJoin like you did, then patching font.mss here to make it use it would be better (at least more aligned with what I've seen in nixpkgs).
In fact, the very principle of get-fonts is not really what we do with nix/nixos (because, as you said elsewhere, it is neither reproducible nor predictable).
There was a problem hiding this comment.
Thanks for that insight. I didn't know it was possible to patch the OSM style to depend directly on the fonts. I thought you needed to pass the fonts to the rendering software separately (such as Nik4 or renderd).
I removed the script.
|
I made 2 remarks, but things are already looking good! Thanks for starting this work :-) |
| lib, | ||
| stdenvNoCC, | ||
| fetchFromGitHub, | ||
| fetchurl, | ||
|
|
||
| carto, | ||
| gdal, | ||
| hanazono, | ||
| nixosTests, | ||
| noto-fonts, | ||
| python3, | ||
| runCommand, | ||
| symlinkJoin, |
There was a problem hiding this comment.
| lib, | |
| stdenvNoCC, | |
| fetchFromGitHub, | |
| fetchurl, | |
| carto, | |
| gdal, | |
| hanazono, | |
| nixosTests, | |
| noto-fonts, | |
| python3, | |
| runCommand, | |
| symlinkJoin, | |
| lib, | |
| stdenvNoCC, | |
| fetchFromGitHub, | |
| fetchurl, | |
| nixosTests, | |
| runCommand, | |
| symlinkJoin, | |
| carto, | |
| gdal, | |
| hanazono, | |
| noto-fonts, | |
| python3, |
| "out" | ||
| "sql" | ||
| "lua" | ||
| "get_external_data" |
There was a problem hiding this comment.
I am still in favor calling it external_data.
| inherit hash; | ||
| }; | ||
| }; | ||
| simplified_water_polygons = mkArchive { |
There was a problem hiding this comment.
What is the purpose of this data ? Isn't it useful mostly for testing ? If that's true, I still lean more towards moving this code to the test.
Another issue of having this code in package is how to update this data over the time (and how often). We might end up with reproducible but very outdated data.
What do you think @autra ?
I think we should usually not package data, especially those data because they change quite often I'm sure. There are exceptions (proj reprojection grids I guess, even though it's probably debatable). Considering that upstream's logic is to let the user downloading data via In short everything that is not in upstream repository should not be packaged imo. However, we can package the script and the default config file (and even patch the script so that it defaults to this config file in the store if necessary). For the derivation output, I'd package all relevant scripts in openstreetmap-carto/scripts in the For other files (styles? fonts if necessary?) I don't mind another output, but I wouldn't mind a folder in the main output, but I admit that the advantages of having different outputs or only one are not totally clear to me |
Add
nik4, a program that can render a map from OpenStreetMap data as an image.openstreetmap-cartois an OpenStreetMap style. It defines how the map looks.Also disable the broken
python3Packages.python-mapniktest, since I need this dependency. It seems to work fine for this use-case.I also wrote a NixOS test to show how everything fits together. If you run it (and wait many minutes), you get a nice map in the output.
I may or may not have gone a bit overboard with the
openstreetmap-cartopackage, where I made it not depend on the internet, so it can run in the NixOS VM test. I also regenerate all automatically generated files that I could find and check that we get the same files. I also separated different parts into their own outputs. This way the closure size should be reduced if you don't need certain parts. For example theget-external-data.pyscript only needs to be run once to import the map into the database and after that, its huge closure (over two gigabytes) is no longer needed (unless you want to update the map).I chose to use the latest git revision of
openstreetmap-cartobecause it supports the flex layout, unlike the latest release.@NixOS/geospatial please review.
I think this is a super cool demo.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.