-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
collision: init at 3.5.0 #242681
collision: init at 3.5.0 #242681
Conversation
@@ -485,6 +485,8 @@ with pkgs; | |||
|
|||
colorpicker = callPackage ../tools/misc/colorpicker { }; | |||
|
|||
collision = callPackage ../applications/misc/collision { }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
}; | ||
|
||
# project dependencies | ||
dependencies = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is a good way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this issue in 1de87e0, take a look at this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
|
||
test: test-binding | ||
- ./bin/spec $(RSPEC_OPTS) | ||
+ sh bin/spec $(RSPEC_OPTS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not patch the shebang instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch was removed in 9c1cd51
, wrapGAppsHook4 | ||
}: | ||
let | ||
gi_crystal = crystal.buildCrystalPackage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not move that into gi-crystal/default.nix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, fixed in 9c1cd51
name = "gi-crystal"; | ||
|
||
src = fetchFromGitHub { | ||
owner = "hugopl"; | ||
repo = "gi-crystal"; | ||
rev = "refs/tags/v0.16.0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it works
doCheck = false; | ||
doInstallCheck = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to disable both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens because
make test-binding
setGI_TYPELIB_PATH="./spec/build"
for binding generator
Just remove this and see if it works.
for dep in *; do | ||
if [ -d "$dep" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for dep in *; do | |
if [ -d "$dep" ]; then | |
for dep in */; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 9c1cd51
c64788e
to
afa0022
Compare
@GeopJr if you'd like, can you help with review from a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
9518473
to
2ae642d
Compare
Hi, @drupol! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
rev = "v${version}"; | ||
hash = "sha256-ij4U8BoSNHpkh5SmvoH5anGB/ZdvmC5zDwJCusR/s/c="; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 is an explanation for this https://danilafe.com/blog/crystal_nix_revisited/ | ||
# Shortly, adding pkg-config to buildInputs along with openssl fixes the issue. | ||
|
||
nativeBuildInputs = [ wrapGAppsHook4 pkg-config ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nativeBuildInputs = [ wrapGAppsHook4 pkg-config ]; | |
nativeBuildInputs = [ wrapGAppsHook4 pkg-config ] | |
++ lib.optionals stdenv.isDarwin [ desktopToDarwinBundle ]; |
@@ -0,0 +1,70 @@ | |||
{ lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ 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 |
, desktopToDarwinBundle | ||
}: | ||
let | ||
gi-crystal = callPackage gi-crystal/default.nix { }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gi-crystal = callPackage gi-crystal/default.nix { }; | |
gi-crystal = callPackage ./gi-crystal/default.nix { }; |
cp -r ${gi-crystal}/* lib/gi-crystal | ||
|
||
chmod +w lib/gi-crystal/src/auto | ||
${gi-crystal}/bin/gi-crystal -o lib/gi-crystal/src/auto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be added to nativeBuildInputs instead
# 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]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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 | |
]; |
40e1697
to
df21f9e
Compare
That's all! After packing the |
Description of changes
Collision - small but useful application, that check hashes for files.
https://github.com/GeopJr/Collision
Things done
sandbox = true
set 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/
)