Add new types #125

GuillaumeGomez · 2018-06-19T20:56:35Z

GuillaumeGomez · 2018-06-19T20:58:26Z

src/auto/subprocess.rs

+
+impl Subprocess {
+    //#[cfg(any(feature = "v2_40", feature = "dox"))]
+    //pub fn new<'a, P: Into<Option<&'a Error>>>(flags: SubprocessFlags, error: P, argv0: &str, : /*Unknown conversion*//*Unimplemented*/Fundamental: VarArgs) -> Subprocess {


I have a crate that we could use to give VarArgs: https://crates.io/crates/va_list-rs (apparently, you can just give a va_list as a VarArgs argument so why not enjoying this?). Should we give it a try?

IMHO it worth to try but not in this PR (or if PR not merged until next release)

Also we discussed that a while ago on IRC and I believe there are some safety concerns there. Have to look again, I can't remember :)

I don't intend to put it in this PR but since I had the chance to talk about it... 😛

Also the newv variant is equivalent and more Rust-y :)

sdroege · 2018-06-19T21:08:24Z

src/auto/inet_address_mask.rs

+pub trait InetAddressMaskExt {
+    fn equal(&self, mask2: &InetAddressMask) -> bool;
+
+    fn get_address(&self) -> Option<InetAddress>;


Can this really return None?

I suppose it cannot.

sdroege · 2018-06-19T21:09:37Z

src/auto/subprocess.rs

+    //}
+
+    #[cfg(any(feature = "v2_40", feature = "dox"))]
+    pub fn newv(argv: &[&std::path::Path], flags: SubprocessFlags) -> Result<Subprocess, Error> {


The argv should be an OsString AFAIU

sdroege · 2018-06-19T21:11:46Z

src/auto/subprocess_launcher.rs

+    //fn spawn(&self, error: &mut Error, argv0: &str, : /*Unknown conversion*//*Unimplemented*/Fundamental: VarArgs) -> Option<Subprocess>;
+
+    #[cfg(any(feature = "v2_40", feature = "dox"))]
+    fn spawnv(&self, argv: &[&std::path::Path]) -> Result<Subprocess, Error>;


OsString as above

sdroege · 2018-06-19T21:12:12Z

src/auto/subprocess_launcher.rs

+    fn set_stdout_file_path<P: AsRef<std::path::Path>>(&self, path: P);
+
+    #[cfg(any(feature = "v2_40", feature = "dox"))]
+    fn setenv<P: AsRef<std::path::Path>, Q: AsRef<std::path::Path>>(&self, variable: P, value: Q, overwrite: bool);


OsString as above (or rather they should all be &OsStr, there's a gir parameter for this)

sdroege · 2018-06-19T21:12:37Z

src/auto/subprocess_launcher.rs

+    fn spawnv(&self, argv: &[&std::path::Path]) -> Result<Subprocess, Error>;
+
+    #[cfg(any(feature = "v2_40", feature = "dox"))]
+    fn take_fd(&self, source_fd: i32, target_fd: i32);


These should be wrappers around RawFd on UNIX and Handle on Windows. All the fd related functions

These fd functions are also unsafe, as you can safely create a random fd (it's just a type alias for an integer) and then having GLib use that will cause problems

I mean they must be marked as unsafe

sdroege · 2018-06-19T21:12:48Z

src/auto/subprocess_launcher.rs

+    fn take_stdout_fd(&self, fd: i32);
+
+    #[cfg(any(feature = "v2_40", feature = "dox"))]
+    fn unsetenv<P: AsRef<std::path::Path>>(&self, variable: P);


sdroege · 2018-06-19T21:13:42Z

Looks generally good except for those comments above. Thanks for working on this :) This will become a good release for GIO!

GuillaumeGomez · 2018-06-19T21:14:31Z

I hope it'll! I need to understand what's happening on the appveyor failure as well. Some duplicate implementations apparently...

EPashkin · 2018-06-19T21:29:04Z

Duplicate trait happened on travis too.
Only objects have buildin Eq (added long ago gtk-rs/glib@10745ad).
Not sure what we can do (IMHO ignoring equals remove generated Eq but it not rigth)

GuillaumeGomez · 2018-06-19T21:33:42Z

I'll test and we'll see. In worst case, I'll just copy/paste the whole file and ignore its generation...

sdroege · 2018-06-20T06:10:06Z

The Eq thing is somewhat related to gtk-rs/glib#247

We need a way to opt-out of the automatic Eq, Debug, etc impls and have custom ones.

sdroege · 2018-06-20T06:16:24Z

I'll test and we'll see. In worst case, I'll just copy/paste the whole file and ignore its generation...

You can ignore the equal function in gir too probably. But it's all not a solution really :)

GuillaumeGomez · 2018-06-22T18:23:24Z

Updated.

sdroege · 2018-06-23T06:42:11Z

src/subprocess_launcher.rs

+use ffi;
+use glib;
+use glib::object::IsA;
+use GtkRawFd;


I don't like this one, and it will also make the API unsafe as you can pass random integers in there now. There's a reason why I said to use the proper traits for this :)

sdroege · 2018-06-23T06:43:17Z

src/subprocess_launcher.rs

+
+pub trait SubprocessLauncherExtManual {
+    #[cfg(any(feature = "v2_40", feature = "dox"))]
+    fn take_fd(&self, source_fd: GtkRawFd, target_fd: GtkRawFd);


These should all take a https://doc.rust-lang.org/nightly/std/os/unix/io/trait.IntoRawFd.html on UNIX and https://doc.rust-lang.org/nightly/std/os/windows/io/trait.IntoRawHandle.html on Windows to be safe (the safety aspect is then moved to the implementors of that trait and people can safely pass various types in there)

See e.g. https://github.com/sdroege/gstreamer-rs/blob/master/gstreamer/src/bus_windows.rs and https://github.com/sdroege/gstreamer-rs/blob/master/gstreamer/src/bus_unix.rs

or https://github.com/gtk-rs/gio/blob/master/src/socket.rs

Was wrong, my link only for sockets

Basically same thing though, yes

Yes, I had unsafe in mind, but I don't get why I didn't think about AsRawFd and AsRawHandle...

Into, not As :) this takes ownership. And it does not have to be unsafe then because the trait impls already take care of that being safe

GuillaumeGomez · 2018-06-23T12:31:47Z

Okay, the safe version is now here!

GuillaumeGomez · 2018-06-23T15:19:40Z

After multiple windows fixes, does it seem good?

EPashkin · 2018-06-23T16:04:41Z

Travis don't think so 😭

EPashkin · 2018-06-23T16:13:54Z

By way, seems manual function in SubProcessLauncher is not_windows only, I can't find it in libgio-2.0-0.dll while g_subprocess_launcher_new is present

GuillaumeGomez · 2018-06-23T16:16:16Z

Another bug in gio I assume... I just switch to cfg(not(windows)) for SubProcessLauncher then?

EPashkin · 2018-06-24T05:02:25Z

Futures bug now in beta too :(
Also errors and warnings in stable https://travis-ci.org/gtk-rs/gio/jobs/395928392 IMHO legit and need fix

sdroege · 2018-06-24T07:09:59Z

Also errors and warnings in stable https://travis-ci.org/gtk-rs/gio/jobs/395928392 IMHO legit and need fix

These are missing imports in the subprocess.rs in this PR (@GuillaumeGomez). And some type mismatches in there. All the other futures code seems to be fine.

Same for the beta problems.

sdroege · 2018-06-24T07:12:18Z

Gir.toml

+    name = "communicate_utf8_async"
+    ignore = true
+    [[object.function]]
+    name = "communicate_utf8_async_future"


You don't have to ignore that one here. It is generated as a result of the non-futures function and does not exist as such in C

sdroege · 2018-06-24T07:14:04Z

src/subprocess.rs

+        use send_cell::SendCell;
+
+        let stdin_buf = stdin_buf.into();
+        let stdin_buf = stdin_buf.map(ToOwned::to_owned);


It already is a String, there's nothing to convert here. But you could make it take a &str and do the copy internally here like you do already

sdroege · 2018-06-24T07:16:21Z

src/subprocess.rs

+        let stdin_buf = stdin_buf.to_glib_none();
+        let cancellable = cancellable.into();
+        let cancellable = cancellable.to_glib_none();
+        let user_data: Box<Box<(R, *mut i8)>> = Box::new(Box::new((callback, stdin_buf.0)));


The stdin_buf has to be the result of to_glib_full() as you want to keep it alive until the trampoline ran and then free it there.

GuillaumeGomez · 2018-06-25T22:36:34Z

Updated!

EPashkin · 2018-06-26T02:02:02Z

src/subprocess_launcher.rs

+#[cfg(any(feature = "v2_40", feature = "dox"))]
+use SubprocessLauncher;
+#[cfg(any(feature = "v2_40", feature = "dox"))]
+use ffi;


This better have same cfg as functions, see warning in appveyor

EPashkin · 2018-06-26T02:02:59Z

src/subprocess_launcher.rs

+#[cfg(any(all(feature = "v2_40", not(windows)), all(feature = "dox", not(windows))))]
+use std::os::unix::io::IntoRawFd;
+#[cfg(all(feature = "dox", windows))]
+pub trait IntoRawFd {}


IMHO better use unix and not(unix) instead

EPashkin · 2018-06-26T02:06:46Z

Looks good, thanks

sdroege · 2018-06-26T06:06:16Z

Go for it then :)

GuillaumeGomez · 2018-06-26T21:38:55Z

I want to update following @EPashkin's comments first. Then I merge. :) Thanks a lot to both of you for your reviews!

EPashkin · 2018-06-27T04:45:25Z

src/subprocess_launcher.rs

+/// Replacement for "real" [`IntoRawFd`] trait for non-unix targets.
+///
+/// [`IntoRawFd`]: https://doc.rust-lang.org/std/os/unix/io/trait.IntoRawFd.html
+pub trait IntoRawFd {}


Your check don't allow doc comments here, you need add it to lgpl-docs, but not sure that it restored in right place.
Also IMHO better not mixing windows and unix and use only unix (in all this file)

EPashkin · 2018-06-27T04:49:17Z

for futures all v0.2.* yanked

sdroege · 2018-06-27T05:53:01Z

for futures all v0.2.* yanked

Great! Complained about that here, let's see what they reply https://users.rust-lang.org/t/futures-0-2-has-been-moved-to-futures-preview/18329/2?u=slomo

sdroege · 2018-06-27T06:32:52Z

Maybe only needs moving to the futures-preview crates instead, I'll check later

EPashkin · 2018-06-27T08:38:51Z

src/subprocess_launcher.rs

@@ -1,36 +1,36 @@
 #[cfg(any(feature = "v2_40", feature = "dox"))]
 use SubprocessLauncher;
-#[cfg(any(all(feature = "v2_40", not(windows)), feature = "dox"))]
+#[cfg(any(all(feature = "v2_40", not(unix)), feature = "dox"))]


This is wrong change, as windows ~= not(unix)

Oups indeed!

GuillaumeGomez · 2018-06-27T09:04:08Z

I fixed conditions.

sdroege · 2018-06-27T09:08:19Z

Currently preparing gio-futures PR, then this can be merged IMHO

EPashkin · 2018-06-27T15:08:27Z

@GuillaumeGomez Docs on windows failed
https://ci.appveyor.com/project/GuillaumeGomez/gio/build/job/ujoevof6anec5ur7

error: string literal with a suffix is invalid
  --> src\subprocess_launcher.rs:12:9
   |
12 | #[doc = "Replacement for "real" [`IntoRawFd`] trait for non-unix targets.
   |         ^^^^^^^^^^^^^^^^^^^^^^
error: unexpected token: `" [`IntoRawFd`] trait for non-unix targets.
[`IntoRawFd`]: https://doc.rust-lang.org/std/os/unix/io/trait.IntoRawFd.html"`
  --> src\subprocess_launcher.rs:12:31
   |
12 |   #[doc = "Replacement for "real" [`IntoRawFd`] trait for non-unix targets.
   |  _______________________________^
13 | |
14 | | [`IntoRawFd`]: https://doc.rust-lang.org/std/os/unix/io/trait.IntoRawFd.html"]
   | |_____________________________________________________________________________^ unexpected token after this
error: Could not document `gio`.

EPashkin · 2018-06-27T15:13:46Z

.. and doc comments will be not accepted anyway it need be added to lgpl-docs

GuillaumeGomez · 2018-06-27T15:37:39Z

This is really annoying. :-/ I'll open a PR to remove it from here.

Add new types

Add new types

ce7a315

GuillaumeGomez commented Jun 19, 2018

View reviewed changes

sdroege reviewed Jun 19, 2018

View reviewed changes

GuillaumeGomez force-pushed the new-types2 branch from c303880 to 7c59612 Compare June 22, 2018 20:47

Improve generation

3ac3424

GuillaumeGomez force-pushed the new-types2 branch from 7c59612 to 3ac3424 Compare June 22, 2018 22:57

sdroege reviewed Jun 23, 2018

View reviewed changes

GuillaumeGomez force-pushed the new-types2 branch 4 times, most recently from 32b8e6e to 32928bb Compare June 23, 2018 14:35

EPashkin mentioned this pull request Jun 23, 2018

Remove callback guard #126

Closed

GuillaumeGomez force-pushed the new-types2 branch from 878b06c to 312e5fa Compare June 23, 2018 21:14

sdroege reviewed Jun 24, 2018

View reviewed changes

New wave of updates

600674f

GuillaumeGomez force-pushed the new-types2 branch from 0215a3d to 600674f Compare June 25, 2018 22:07

EPashkin reviewed Jun 26, 2018

View reviewed changes

Small improvements to avoid warnings and add a bit of doc

af088be

EPashkin reviewed Jun 27, 2018

View reviewed changes

Replace all windows conditions

2d7017a

GuillaumeGomez force-pushed the new-types2 branch from 6df4ce2 to 2d7017a Compare June 27, 2018 09:03

GuillaumeGomez merged commit a4eee77 into gtk-rs:master Jun 27, 2018

GuillaumeGomez deleted the new-types2 branch June 27, 2018 11:47

vhdirk pushed a commit to vhdirk/gio-rs that referenced this pull request Jan 16, 2019

Merge pull request gtk-rs#125 from GuillaumeGomez/new-types2

9f12c98

Add new types

Add new types #125

Add new types #125

Conversation

GuillaumeGomez commented Jun 19, 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

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

sdroege commented Jun 19, 2018

GuillaumeGomez commented Jun 19, 2018 • edited Loading

EPashkin commented Jun 19, 2018

GuillaumeGomez commented Jun 19, 2018

sdroege commented Jun 20, 2018

sdroege commented Jun 20, 2018

GuillaumeGomez commented Jun 22, 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

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

GuillaumeGomez commented Jun 23, 2018

GuillaumeGomez commented Jun 23, 2018

EPashkin commented Jun 23, 2018

EPashkin commented Jun 23, 2018

GuillaumeGomez commented Jun 23, 2018

EPashkin commented Jun 24, 2018

sdroege commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EPashkin commented Jun 26, 2018

sdroege commented Jun 26, 2018

GuillaumeGomez commented Jun 26, 2018

Choose a reason for hiding this comment

EPashkin commented Jun 27, 2018

sdroege commented Jun 27, 2018

sdroege commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jun 27, 2018

sdroege commented Jun 27, 2018

EPashkin commented Jun 27, 2018

EPashkin commented Jun 27, 2018

GuillaumeGomez commented Jun 27, 2018

GuillaumeGomez commented Jun 19, 2018 •

edited

Loading