collision: init at 3.5.0#242681
Conversation
Janik-Haag
added
the
12.first-time contribution
ofborg
bot
added
8.has: package (new)
pkgs/top-level/all-packages.nix
Outdated
There was a problem hiding this comment.
move this above colmena for alphabetical order
|
Also just FYI, as I see you used the github interface to accept the review changes, you will need to squash and force-push the commits for this PR at the end before it gets approved (see https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#writing-good-commit-messages) |
NobbZ
left a comment
NobbZ
left a comment
There was a problem hiding this comment.
Please make also sure, that before merging you squash everything together into exactly 2 commits:
- add yourself as maintainer, message: "maintainers: Add sund3RRR as a maintainer"
- everything related to the package: "collision: init at 3.5.0"
You also might want to add a meta.mainProgram.
There was a problem hiding this comment.
As far as I can see while skimming other usages of crystal.buildCrystalPackage, they seem to provide a generated shardsFile, which seems to be responsible for the deps.
There was a problem hiding this comment.
I tried to build this package with the dependencies generated by shards.nix, but the gi-crystal dependency needs to be built. There is no documentation for crystal.buildCrystalPackage and I don't know how to specify a dependency building phase for it.
There was a problem hiding this comment.
In that case it might make sense to make that its completely own derivation, which you use as a nativeBuildInputs, as from what I get now from rereading the comments and your reply here, it seems to be some kind of build tool that needs to run on the build host natively, and does not necessarily need to be ran on the target host?
There was a problem hiding this comment.
Yes, it is a good way.
There was a problem hiding this comment.
Fixed this issue in 1de87e0, take a look at this.
There was a problem hiding this comment.
I copy not only the gi-crystal, but also all other dependencies for the gi-crystal itself.
crystal.buildCrystalPackage collect dependencies specified in shards.nix into a separated drv and symlinks it from lib/ directory. But when i run gi-crystal it cannot see dependencies linked by symlink. Perhaps there is a check for a directory, and since the file is a link to the directory, and not the directory itself, gi-crystal generates bindings without these dependencies, especially without libadwaita, which is needed for Collision.
I would pack the gi-crystal before packing the collision, but I don’t know how to solve the problem of generating bindings, because the gi-crystal does not see the dependencies attached by a symbolic link.
There was a problem hiding this comment.
FWIW, gi-crystal searches for binding files in the project root (including its dependencies) and generates bindings for them based on their gir files. By default it searches for gir files under /usr/share/gir-1.0/ but you can pass : separated paths to the GI_TYPELIB_PATH env var. From a quick look, I do not see anything about not following symlinks (File#exists? is used but it follows symlinks)
There was a problem hiding this comment.
Thank you for your help, @GeopJr. It seems that there are no problems with the detection of typelibs, the main problem is to find binding.yml
I write a simple bash script, that find and print all binding.yml files in directory.
ls -la
start_directory="./"
found_files=$(find "$start_directory" -type f -name "binding.yml" 2>/dev/null)
And it prints
lrwxrwxrwx 1 nixbld nixbld 59 Jul 14 13:24 gio -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/gio
lrwxrwxrwx 1 nixbld nixbld 60 Jul 14 13:24 gtk4 -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/gtk4
lrwxrwxrwx 1 nixbld nixbld 64 Jul 14 13:24 harfbuzz -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/harfbuzz
lrwxrwxrwx 1 nixbld nixbld 66 Jul 14 13:24 libadwaita -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/libadwaita
lrwxrwxrwx 1 nixbld nixbld 61 Jul 14 13:24 pango -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/pango
Found files:
./src/bindings/g_lib/binding.yml
./src/bindings/g_object/binding.yml
Only if i specify directory like start_directory="lib/gio/src", it will find gio's binding.yml
lrwxrwxrwx 1 nixbld nixbld 59 Jul 14 13:25 gio -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/gio
lrwxrwxrwx 1 nixbld nixbld 60 Jul 14 13:25 gtk4 -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/gtk4
lrwxrwxrwx 1 nixbld nixbld 64 Jul 14 13:25 harfbuzz -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/harfbuzz
lrwxrwxrwx 1 nixbld nixbld 66 Jul 14 13:25 libadwaita -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/libadwaita
lrwxrwxrwx 1 nixbld nixbld 61 Jul 14 13:25 pango -> /nix/store/dkwc4p77z9aw9fkgi6z6gcc4sbrxdjh6-crystal-lib/pango
Found files:
lib/gio/src/bindings/gio/binding.yml
I looked at the gi-crystal sources and it looks like it is looking for these files in some similar way, so it cannot find them.
private def find_bindings : Array(String)
find_pattern = Path.new(project_dir, "**/binding.yml").normalize
Dir[find_pattern]
end
There was a problem hiding this comment.
Just look at how many bindings gi-crystal finds when i copy dependencies manually
Dependencies are satisfied
Building: gi-crystal
Resolving dependencies
Writing shard.lock
info - Starting at 2023-07-14 13:42:33 UTC, project dir: /build/source
info - Gi-Crystal version 0.16.0, built with Crystal 1.8.2.
info - Generating bindings at /build/source/src/auto
info - Using binding config at /build/source/lib/pango/src/bindings/pango/binding.yml
info - Using binding config at /build/source/lib/harfbuzz/src/bindings/harfbuzz/binding.yml
info - Using binding config at /build/source/lib/gio/src/bindings/gio/binding.yml
info - Using binding config at /build/source/lib/gtk4/src/bindings/gdk/binding.yml
info - Using binding config at /build/source/lib/gtk4/src/bindings/gtk/binding.yml
info - Using binding config at /build/source/lib/gtk4/src/bindings/gsk/binding.yml
info - Using binding config at /build/source/lib/libadwaita/src/bindings/binding.yml
info - Using binding config at /build/source/src/bindings/g_lib/binding.yml
info - Using binding config at /build/source/src/bindings/g_object/binding.yml...
And how many without it:
Dependencies are satisfied
Building: gi-crystal
Resolving dependencies
Writing shard.lock
info - Starting at 2023-07-14 13:24:41 UTC, project dir: /build/source
info - Gi-Crystal version 0.16.0, built with Crystal 1.8.2.
info - Generating bindings at /build/source/src/auto
info - Using binding config at /build/source/src/bindings/g_lib/binding.yml
info - Using binding config at /build/source/src/bindings/g_object/binding.yml
glibPreInstallPhase
There was a problem hiding this comment.
And of course i get compiler error:
In src/collision.cr:2:1
2 | require "libadwaita"
^
Error: Bindings for Adw-1 not yet generated, run ./bin/gi-crystal first.
|
\cc @peterhoeg @manveru and @Br1ght0ne You are randomly choosen samples of maintainers with a crytalpackage and I'd like to see your review as well. |
sund3RRR
changed the title
There was a problem hiding this comment.
Why not patch the shebang instead?
There was a problem hiding this comment.
Why not move that into gi-crystal/default.nix?
There was a problem hiding this comment.
| name = "gi-crystal"; | |
| src = fetchFromGitHub { | |
| owner = "hugopl"; | |
| repo = "gi-crystal"; | |
| rev = "refs/tags/v0.16.0"; | |
| pname = "gi-crystal"; | |
| version = "0.16.0"; | |
| src = fetchFromGitHub { | |
| owner = "hugopl"; | |
| repo = "gi-crystal"; | |
| rev = "refs/tags/v${version}"; |
There was a problem hiding this comment.
error: undefined variable 'version'
at /home/sunder/repos/nixpkgs/pkgs/applications/misc/collision/default.nix:20:27:
19| repo = "gi-crystal";
20| rev = "refs/tags/v${version}";
| ^
21| hash = "sha256-ij4U8BoSNHpkh5SmvoH5anGB/ZdvmC5zDwJCusR/s/c=";
maybe if move this into gi-crystal/default.nix, it might work.
There was a problem hiding this comment.
Do we really need to disable both?
There was a problem hiding this comment.
Without doCheck = false;:
> info - Using binding config at /build/source/src/bindings/g_object/binding.yml
> info - Using binding config at /build/source/spec/libtest_binding.yml
> fatal - Typelib file for namespace 'Pango', version '1.0' not found
> make: *** [Makefile:13: test-binding] Error 1
This happens because make test-binding set GI_TYPELIB_PATH="./spec/build" for binding generator
test-binding: libtest generator
GI_TYPELIB_PATH="./spec/build" LIBRARY_PATH="./spec/build" LD_LIBRARY_PATH="./spec/build" ./bin/gi-crystal spec/libtest_binding.yml -o src/auto --doc
Without doInstallCheck = false;:
> In /nix/store/3ba2q00y5jpjxbwcg6m9fnr3ixj3y2kx-gi-crystal/bin/compare-api:90:14
>
> 90 | if Process.fork
> ^---
> Warning: Deprecated Process.fork. Fork is no longer supported.
>
> A total of 1 warnings were found.
> /nix/store/blpvf60m29q02c0lc5fyhim30ma4y1vv-stdenv-linux/setup: line 1597: [: /nix/store/3ba2q00y5jpjxbwcg6m9fnr3ixj3y2kx-gi-crystal/bin/format: unary operator expected
> /nix/store/3ba2q00y5jpjxbwcg6m9fnr3ixj3y2kx-gi-crystal/bin/format: line 4: clang-format: not found
I don't know what is it and just disable doInstallCheck....
There was a problem hiding this comment.
This happens because
make test-bindingsetGI_TYPELIB_PATH="./spec/build"for binding generator
Just remove this and see if it works.
There was a problem hiding this comment.
| for dep in *; do | |
| if [ -d "$dep" ]; then | |
| for dep in */; do |
sund3RRR
force-pushed
the
collision-package-branch
branch
2 times, most recently
from
c64788e to
afa0022
Compare
ofborg
bot
added
10.rebuild-darwin: 0
|
@GeopJr if you'd like, can you help with review from a |
ofborg
bot
added
10.rebuild-darwin: 1-10
drupol
left a comment
drupol
left a comment
There was a problem hiding this comment.
Hi !
I made a few comments, let me know if you need help implementing them.
I also set the PR in draft, please, remove the draft status as soon as you think everything is fixed so I can give it another review.
Thanks in advance!
sund3RRR
force-pushed
the
collision-package-branch
branch
from
9518473 to
2ae642d
Compare
|
Hi, @drupol! |
drupol
left a comment
drupol
left a comment
There was a problem hiding this comment.
Thanks for addressing the remarks!
|
I notice that the Darwin build is failing... Is it an expected behavior? |
|
I haven't tested on Darwin yet. I think I won't test on darwin because I don't have a macbook :( |
|
It’s failing to link on Darwin because gi-crystal is mangling the library names for pkg-config (e.g., gio should be Before marking it broken, I want to see if I can fix the generator and add |
|
gi-crystal needs patched to work correctly with libraries in the nix store. I’m surprised this builds on Linux. If the package name starts with For example, libadwaita is installed in this path on my system: What it should be finding is |
There was a problem hiding this comment.
| # Make sure gi-crystal picks up the name of the so or dylib and not the leading nix store path | |
| # when the package name happens to start with “lib”. | |
| patches = [ ./store-friendly-library-name.patch ]; | |
There was a problem hiding this comment.
The contents of the patch:
diff -ur a/src/generator/lib_gen.cr b/src/generator/lib_gen.cr
--- a/src/generator/lib_gen.cr 1969-12-31 17:00:01.000000000 -0700
+++ b/src/generator/lib_gen.cr 2023-07-14 11:48:41.509397114 -0600
@@ -10,7 +10,7 @@
private def libraries : Array(String)
namespace.shared_libraries.map do |library|
- library[/lib([^\/]+)\.(?:so|.+?\.dylib).*/, 1]
+ library[/(?:\/[^\/]*)+\/lib([^\/]+)\.(?:so|.+?\.dylib).*/, 1]
end
end
There was a problem hiding this comment.
| nativeBuildInputs = [ wrapGAppsHook4 pkg-config ]; | |
| nativeBuildInputs = [ wrapGAppsHook4 pkg-config ] | |
| ++ lib.optionals stdenv.isDarwin [ desktopToDarwinBundle ]; |
There was a problem hiding this comment.
| { lib | |
| { stdenv | |
| , lib |
|
With those suggested changes, Collision builds for me. It even has a nice .app bundle on Darwin with an icon. |
|
I made a gi-crystal source patch, it's almost usable in nix. (Warning: build is broken on this commit) 99939f5 Another fix is to set project dir outside of gi-crystal binary. Just set Now we can call gi-crystal binary and generate bindings easily without copying. But there is one problem. and now i'm stuck at this point. |
|
I tried different methods and came to the conclusion that in the current version, without copying If you remove the And then generate all bindings that you need: I don't think there is any other way to do it better. @GeopJr, can you review I can split this PR into |
|
I packed the |
There was a problem hiding this comment.
| gi-crystal = callPackage gi-crystal/default.nix { }; | |
| gi-crystal = callPackage ./gi-crystal/default.nix { }; |
There was a problem hiding this comment.
That should be added to nativeBuildInputs instead
There was a problem hiding this comment.
| # Make sure gi-crystal picks up the name of the so or dylib and not the leading nix store path | |
| # when the package name happens to start with “lib”. | |
| patches = [ ./store-friendly-library-name.patch ./find_bindings.patch]; | |
| patches = [ | |
| # Make sure gi-crystal picks up the name of the so or dylib and not the leading nix store path | |
| # when the package name happens to start with “lib”. | |
| ./store-friendly-library-name.patch | |
| ./find_bindings.patch | |
| ]; |
sund3RRR
force-pushed
the
collision-package-branch
branch
from
40e1697 to
df21f9e
Compare
|
That's all! After packing the |
8 participants
Description of changes
Collision - small but useful application, that check hashes for files.
https://github.com/GeopJr/Collision
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)