romgrk / node-gtk

GTK+ bindings for NodeJS (via GObject introspection)
MIT License
494 stars 41 forks source link

fix(value): don't assume that zero-terminated array is of type gchar** #313

Closed peat-psuwit closed 2 years ago

peat-psuwit commented 2 years ago

The array's element type can be arbitrary, so we need to check the value with the correct type. The long switch and a template function is used to do this job.

We can't just memcmp() with item_size, because one of the type is float, and we can't assume a 0.0 float is the same as 0 integer.

A test, based on how I discovered this, is added.

peat-psuwit commented 2 years ago

I don't understand the test failure on macos.

     Failed: OUT-array (zero-terminated, of guint8)
TypeError: Not enough arguments; expected 2, have 1
    at /Users/runner/work/node-gtk/node-gtk/tests/conversion__array.js:56:23
...

The annotation of the function is:

 * g_spawn_command_line_sync:
 * @command_line: (type filename): a command line
 * @standard_output: (out) (array zero-terminated=1) (element-type guint8) (optional): return location for child output
 * @standard_error: (out) (array zero-terminated=1) (element-type guint8) (optional): return location for child errors
 * @wait_status: (out) (optional): return location for child wait status, as returned by waitpid()
 * @error: return location for errors

So the JS version should take only 1 argument, the command_line. Besides, the test works on my machine running Ubuntu 20.04. Maybe it comes from that glib on the macos one is 2.70. I don't have a Mac that can debug this.

romgrk commented 2 years ago

Can't see the tests, can you rebase on master to fetch the updated workflow yaml file?

For the issue, maybe you could detect the OS or glib version, and provide the missing argument depending on that?

peat-psuwit commented 2 years ago

Can't see the tests, can you rebase on master to fetch the updated workflow yaml file?

Done. Please see the result.

For the issue, maybe you could detect the OS or glib version, and provide the missing argument depending on that?

The problem is, I don't know what I'm supposed to pass in as the second argument. Without a Mac, I can't take a look at the Gir file or use a debugger to find out what the second argument is.

romgrk commented 2 years ago

You can add the following line somewhere in the CI script to see the metadata for that function:

node -p "require('./lib/inspect').parseNamespace('GLib').spawn_command_line_sync"
peat-psuwit commented 2 years ago
    ArgInfo {
      infoType: 'arg',
      name: 'exit_status',
      parent: [Circular *1],
      typeName: 'gint32',
      type: [TypeInfo],
      direction: 'IN',
      transfer: 'NOTHING'
    }

Huh?

peat-psuwit commented 2 years ago

So, I got myself a macOS VM, and discovered a problem with Homebrew's gobject-introspection bottle. Put a workaround to rebuild that from source, and now the CI is green.

romgrk commented 2 years ago

Thanks :)

@binyamin: I'm very inconsistent in my presence on github due to lack of time :( As you have write access to the repo, I bless you with the power to merge PRs if you feel like they're complete. I can also give you access to NPM so you can publish in case I'm not around. @peat-psuwit if you want write access the above also applies to you as you've been doing good contributions to the project.