navit: add supports, xkdb, and many fixes #45657

bignaux · 2018-08-26T18:37:09Z

Motivation for this change

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

timokau · 2018-08-26T18:45:02Z

pkgs/applications/misc/navit/default.nix

 stdenv.mkDerivation rec {
  name = "navit-${version}";
  version = "0.5.1";

  src = fetchFromGitHub {
    owner = "navit-gps";
    repo = "navit";
-    rev = "v${version}";
+    rev = version;


Why did you change this? Looks like they do tag their releases with v${version}: https://github.com/navit-gps/navit/releases

timokau · 2018-08-26T18:46:15Z

pkgs/applications/misc/navit/CMakeLists.txt.patch

+ CHECK_INCLUDE_FILES(stdint.h HAVE_STDINT_H)
+ CHECK_INCLUDE_FILES(byteswap.h HAVE_BYTESWAP_H)
+ CHECK_LIBRARY_EXISTS(gypsy gypsy_control_get_default "" GYPSY_FOUND)
+-CHECK_INCLUDE_FILES(libspeechd.h HAVE_LIBSPEECHD)


Why is this patch necessary? It should either indicate a packaging failure on our side or an insufficiently flexible build script upstream. Either way it should be fixed.

you think we should symlink the headers here like other distros ? i wonder it was the new place and i've fix it.

Are other distros doing that?

or copy https://packages.debian.org/sid/amd64/libspeechd-dev/filelist
@berce : what do you think ?

I feel like navit should be able to handle upstream behaviour. Or maybe there is some configure option for libspeechd that tells it to make those symlinks.

However if making those links is a de-facto standard among distributions, we should do it too. And we should notify upstream of that standard so that they can include the links in future releases.

I still think this should be investigated, even though Mic92 merged already.

timokau · 2018-08-26T18:47:10Z

pkgs/applications/misc/navit/default.nix

-  #hardeningDisable = [ "format" ];
-  NIX_CFLAGS_COMPILE = [ "-I${SDL.dev}/include/SDL" ];
+  patches = [ ./CMakeLists.txt.patch ];
+  hardeningDisable = [ "format" ];


Why is this necessary again?

i don't understand well and i restore from the original navit since it was not failing.

But it was commented out previously.

timokau · 2018-08-26T18:47:28Z

pkgs/applications/misc/navit/default.nix

+  patches = [ ./CMakeLists.txt.patch ];
+  hardeningDisable = [ "format" ];
+
+  NIX_CFLAGS_COMPILE = [ ]


Why the empty list?

timokau · 2018-08-26T18:49:40Z

pkgs/applications/misc/navit/default.nix

    # emulate DSAMPLE_MAP
-    mkdir -p $out/share/navit/maps/maps
+    mkdir -p $out/share/navit/maps/


Please quote $out

Quoting is not necessary. We do not support spaces in nix store path. Even if would fix all our code we cannot fix all upstream projects. Therefore we keep code simple and do not quote at all.

I didn't know that we do not support that, do you have a source on that? I agree that it would probably lead to a multitude of problems and be impractical at the moment, but that doesn't mean that we shouldn't try to do the "right" thing as much as possible.

I also disagree that leaving out the quotes makes the code simpler. It makes it necessary to reason about weather or not this variable may have spaces. It is far simpler in my opinion to always quote variables except in the cases where splitting on whitespace is explicitly wanted. Otherwise it always comes back to bite you in the future. Also quoting unconditionally would allow us to run shellcheck on scripts, as was proposed somewhere before.

So I don't see any disadvantage to quoting but quite some advantages.

Nix does not allow spaces in its grammar for paths: https://github.com/NixOS/nix/blob/master/src/libexpr/lexer.l#L92
I don't see this will changing in future.

Okay, good to know. Thanks. My other points are still valid though.

At the moment omitting quotation is common sense:

$ ag 'wrapProgram $out' | wc -l 596 $ ag 'wrapProgram "$out' | wc -l 179

There is also sometimes pitfalls of quoting twice.

If you want to change direction of how we handle quoting in builder scripts it is better to open a new issue. Because otherwise our reviews will become inconsistent which can be annoying for contributors.

I don't think "the way it is commonly done" and "common sense" are necessarily the same thing.

Do do that consistently we would need a style guide and automated tooling. Thats a way bigger problem to tackle.

timokau · 2018-08-26T18:50:05Z

pkgs/applications/misc/navit/default.nix


  # we dont want blank screen by defaut
  postInstall = ''
+    # copy a better configuration for nixos desktop
+    cp "${./navit.xml}" $out/share/navit/


Please quote $out/share/navit

timokau · 2018-08-26T18:50:27Z

pkgs/applications/misc/navit/default.nix

    # emulate DSAMPLE_MAP
-    mkdir -p $out/share/navit/maps/maps
+    mkdir -p $out/share/navit/maps/
    bzcat "${sample_map}" | $out/bin/maptool "$out/share/navit/maps/osm_bbox_11.3,47.9,11.7,48.2.bin"


see above above.

timokau · 2018-08-26T18:51:40Z

pkgs/applications/misc/navit/default.nix

    done
+    wrapProgram $out/bin/navit \
+      --prefix PATH : ${makeBinPath ([ espeak xkbd ] ++ optional speechdSupport [ speechd ]) }


Why is [ espeak xkbd ] necessary now? Should that be optional?

i don't make that optional since i think a gps navitation has to have it. It's forbidden to look at screen when we drive here. i can make it optional, but previously, i didn't.

Well if it is an optional dependency according to upstream, I think it is better practice to make it optional. If you think it is essential, you can then default the optional dependency to true.

GrahamcOfBorg · 2018-08-26T18:52:29Z

Success on x86_64-linux (full log)

Attempted: navit

Partial log (click to expand)

shrinking /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1/lib64/navit/gui/libgui_internal.so
shrinking /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1/lib64/navit/osd/libosd_core.so
shrinking /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1/bin/navit
shrinking /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1/bin/maptool
gzipping man pages under /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1/share/man/
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1/lib64  /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1/bin
patching script interpreter paths in /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1
checking for references to /build in /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1...
moving /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1/lib64/* to /nix/store/clvf0xajfxvqhkp57ggpxrvnfmfjj5dx-navit-0.5.1/lib

timokau · 2018-08-26T18:52:33Z

pkgs/applications/misc/navit/navit.xml

@@ -0,0 +1,7324 @@
+<?xml version="1.0" encoding="UTF-8"?><!--


Did you submit this by accident?

i'd like to introduce a better config for nixos.

Better than what? Does that really need a 7k line xml file? Where does that file come from?

We cannot accept that large files as it would affect the channel and repository size.

GrahamcOfBorg · 2018-08-26T18:54:35Z

Success on aarch64-linux (full log)

Attempted: navit

Partial log (click to expand)

shrinking /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1/lib64/navit/graphics/libgraphics_gtk_drawing_area.so
shrinking /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1/lib64/navit/font/libfont_freetype.so
shrinking /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1/bin/maptool
shrinking /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1/bin/navit
gzipping man pages under /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1/share/man/
strip is /nix/store/y4ymnvgxygpq05h03kyzbj572zmh6zla-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1/lib64  /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1/bin
patching script interpreter paths in /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1
checking for references to /build in /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1...
moving /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1/lib64/* to /nix/store/9aa7j8zgvydy1dcpd8433zq0w8fa58cl-navit-0.5.1/lib

GrahamcOfBorg · 2018-08-27T10:20:35Z

Success on x86_64-linux (full log)

Attempted: navit

Partial log (click to expand)

shrinking /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1/lib64/navit/gui/libgui_internal.so
shrinking /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1/lib64/navit/osd/libosd_core.so
shrinking /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1/bin/navit
shrinking /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1/bin/maptool
gzipping man pages under /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1/share/man/
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1/lib64  /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1/bin
patching script interpreter paths in /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1
checking for references to /build in /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1...
moving /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1/lib64/* to /nix/store/3lhnsjx8si12vndbfnqsnmm3hng8aq5g-navit-0.5.1/lib

GrahamcOfBorg · 2018-08-27T10:35:11Z

Success on aarch64-linux (full log)

Attempted: navit

Partial log (click to expand)

shrinking /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1/lib64/navit/graphics/libgraphics_gtk_drawing_area.so
shrinking /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1/lib64/navit/font/libfont_freetype.so
shrinking /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1/bin/maptool
shrinking /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1/bin/navit
gzipping man pages under /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1/share/man/
strip is /nix/store/y4ymnvgxygpq05h03kyzbj572zmh6zla-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1/lib64  /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1/bin
patching script interpreter paths in /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1
checking for references to /build in /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1...
moving /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1/lib64/* to /nix/store/0yjmwrgn39jh2i16c5gs85av4b32mydz-navit-0.5.1/lib

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Aug 26, 2018

timokau requested changes Aug 26, 2018

View reviewed changes

timokau reviewed Aug 26, 2018

View reviewed changes

navit: add supports, xkdb, and many fixes

866f9bb

bignaux force-pushed the navit branch from 3a1d42f to 866f9bb Compare August 27, 2018 10:05

Mic92 merged commit 718ee42 into NixOS:master Aug 27, 2018

bignaux deleted the navit branch August 27, 2018 10:43

timokau mentioned this pull request Sep 6, 2018

nixos/weechat: add module #45728

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

navit: add supports, xkdb, and many fixes #45657

navit: add supports, xkdb, and many fixes #45657

bignaux commented Aug 26, 2018 •

edited

timokau Aug 26, 2018

bignaux Aug 26, 2018

timokau Aug 26, 2018

bignaux Aug 26, 2018

timokau Aug 26, 2018

bignaux Aug 27, 2018 •

edited

timokau Aug 27, 2018

timokau Aug 27, 2018

timokau Aug 26, 2018

bignaux Aug 26, 2018

timokau Aug 26, 2018

timokau Aug 26, 2018

timokau Aug 26, 2018

Mic92 Aug 27, 2018

timokau Aug 27, 2018

Mic92 Aug 27, 2018 •

edited

timokau Aug 27, 2018

Mic92 Aug 27, 2018

timokau Aug 27, 2018

timokau Aug 26, 2018

Mic92 Aug 27, 2018

timokau Aug 26, 2018

Mic92 Aug 27, 2018

timokau Aug 26, 2018

bignaux Aug 26, 2018

timokau Aug 26, 2018

GrahamcOfBorg commented Aug 26, 2018

timokau Aug 26, 2018

bignaux Aug 26, 2018

timokau Aug 26, 2018

Mic92 Aug 27, 2018

GrahamcOfBorg commented Aug 26, 2018

GrahamcOfBorg commented Aug 27, 2018

GrahamcOfBorg commented Aug 27, 2018

		@@ -0,0 +1,7324 @@
		<?xml version="1.0" encoding="UTF-8"?><!--

navit: add supports, xkdb, and many fixes #45657

navit: add supports, xkdb, and many fixes #45657

Conversation

bignaux commented Aug 26, 2018 • edited

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bignaux Aug 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Aug 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrahamcOfBorg commented Aug 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrahamcOfBorg commented Aug 26, 2018

GrahamcOfBorg commented Aug 27, 2018

GrahamcOfBorg commented Aug 27, 2018

bignaux commented Aug 26, 2018 •

edited

bignaux Aug 27, 2018 •

edited

Mic92 Aug 27, 2018 •

edited