Skip to content
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

Fixed incomplete type declaration #167

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BigBoyBarney
Copy link
Contributor

@BigBoyBarney BigBoyBarney commented Dec 27, 2024

This fixes the issue I opened in the GTK4.cr repo.
I have no idea if this breaks anything else.
FWIW the specs with make test passed 🤷

@hugopl
Copy link
Owner

hugopl commented Dec 27, 2024

If it's passing on CI os a good sign, but would be nice to add a test for the use case it fixes. I'm without access to a computer for few days, but can help you with this commenting from my phone.

The ideia is to create a method in libtest that mimic the buggy method found in GTK4, then add a test that call it and check if everything was ok.

@BigBoyBarney
Copy link
Contributor Author

BigBoyBarney commented Dec 28, 2024

Libtest contains C code. Normally I would write a spec like

it "correctly splices an array of objects into a ListModel" do
  list_store = Gio::ListStore.new(Gtk::Button.g_type)
  one = Gtk::Button.new
  items = [one]
  list_store.splice(position: 0_u32, n_removals: 0_u32, additions: items)
end

But I suppose that's not what you had in mind :D

@hugopl
Copy link
Owner

hugopl commented Dec 28, 2024

If you can reproduce the issue with modules included in gi-crystal is even better!

Sometimes it's not possible because the use case is found just in GTK, so a libtest code is needed since gi-crystal can't depend on GTK.

@BigBoyBarney
Copy link
Contributor Author

BigBoyBarney commented Dec 28, 2024

Ahhh, okay, alright then!
The issue was an argument type mismatch, so it was a compile-time error in Crystal.

Sorry for the silly questions, but I can't write the aforementioned spec, because it can't find things like Gtk::Button. 🤣, even though it's in the generated bindings auto/gtk4. What's the intended way of doing this?

Also, since the above example is an argument type mismatch at compile time, the spec wouldn't even be able to "run" if it failed. Not sure if that's a good thing or not 🤣

@hugopl
Copy link
Owner

hugopl commented Dec 28, 2024

So it's the case for a libtest implementation. I'm moving right now, without Internet and my office is inside boxes 😅, if I find some time I will check the issue in detail and can tell the directions to reproduce the issue in libtest.

@hugopl
Copy link
Owner

hugopl commented Dec 28, 2024

You can use Test::Subject instead of Gtk::Button and check if it also fail.

@BigBoyBarney
Copy link
Contributor Author

BigBoyBarney commented Dec 28, 2024

The following spec

describe "ListStore" do
  it "can splice an array" do
    list_store = Gio::ListStore.new(Test::Subject.g_type)
    one = Test::Subject.new
    items = [one]
    list_store.splice(position: 0_u32, n_removals: 0_u32, additions: items)
  end
end

fails on current main with the compile-time errors:

In spec/list_spec.cr:65:18

 65 | list_store.splice(position: 0_u32, n_removals: 0_u32, additions: items)
                 ^-----
Error: instantiating 'Gio::ListStore#splice(position: UInt32, n_removals: UInt32, additions: Array(Test::Subject))'


In src/auto/gio-2.0/list_store.cr:339:67

 339 | LibGio.g_list_store_splice(to_unsafe, position, n_removals, additions, n_additions)
                                                                   ^--------
Error: argument 'additions' of 'LibGio#g_list_store_splice' must be Pointer(LibGObject::Object), not Pointer(Pointer(Void))
make: *** [Makefile:4: test] Error 1

And passes in the PR. So in that regard, it "works" as proof, but the spec run erroring out on the main branch (without the PR) at compile time and aborting instead of failing that specific test isn't great.

@hugopl
Copy link
Owner

hugopl commented Dec 28, 2024

Add the test in this same PR that has the fix, so we merge all together.

@BigBoyBarney
Copy link
Contributor Author

BigBoyBarney commented Dec 28, 2024

It fails the CI test but passes locally 🤔
image

@hugopl
Copy link
Owner

hugopl commented Dec 28, 2024

It fail just when compiling with -Ddebugmemory, so it probably pass by some coincidence when not using this flag that just print to stdout when a object is garbage collected.

@BigBoyBarney
Copy link
Contributor Author

I edited the bin/spec script to include -Ddebug_memory like

#!/bin/env sh

export BUILD_DIR="$(pwd)/.."
export BUILD_DIR=$(dirname $BUILD_DIR)

# Let the linker know where libtest is
export GI_TYPELIB_PATH="$BUILD_DIR/spec/build"
export LIBRARY_PATH="$BUILD_DIR/spec/build"
export LD_LIBRARY_PATH="$BUILD_DIR/spec/build"
# export G_DEBUG=fatal-warnings

crystal spec -d --error-trace -Ddebugmemory $@

and it still passes locally.
image

Is there something else I should enable to get the same test that fails on CI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LibGio#g_list_store_splice argument error
2 participants