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

OsString support and regen #330

Merged
merged 6 commits into from May 10, 2018

Conversation

Projects
None yet
4 participants
@EPashkin
Member

EPashkin commented Apr 30, 2018

Part of gtk-rs/gir#592

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 30, 2018

Member

Currently only tests added.
Found that glib have utils.rs that contains only global function and better be renamed to functions.rs.
g_environ_*env IMHO unusable: works on memory copy, in it C it want result of g_get_environ, but in rust get_environ returns Vec<std::path::PathBuf> while functions want &[&std::path::Path].

Member

EPashkin commented Apr 30, 2018

Currently only tests added.
Found that glib have utils.rs that contains only global function and better be renamed to functions.rs.
g_environ_*env IMHO unusable: works on memory copy, in it C it want result of g_get_environ, but in rust get_environ returns Vec<std::path::PathBuf> while functions want &[&std::path::Path].

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 9, 2018

Member

@GuillaumeGomez, @sdroege This almost final version: only need update gir.

I need your options for my decision:
GLib have [get_environ](https://github.com/gtk-rs/glib/blob/master/src/auto/functions.rs#L475-L479, https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-get-environ) and 3 functions that used it result.
I can't find usage for environ_(un)setenv in rust and even in original C, as seems no functions that accept changed list of environment string in form ["key=value"].
So I decided remove environ_(un)setenv completely and write manual version for environ_getenv so it can accept result from get_environ directly.

Member

EPashkin commented May 9, 2018

@GuillaumeGomez, @sdroege This almost final version: only need update gir.

I need your options for my decision:
GLib have [get_environ](https://github.com/gtk-rs/glib/blob/master/src/auto/functions.rs#L475-L479, https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-get-environ) and 3 functions that used it result.
I can't find usage for environ_(un)setenv in rust and even in original C, as seems no functions that accept changed list of environment string in form ["key=value"].
So I decided remove environ_(un)setenv completely and write manual version for environ_getenv so it can accept result from get_environ directly.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 9, 2018

Member

If it's just a wrapper above the C utils, there is no point to bind it since Rust has its own methods to interact with environment. I think you took the good decision by removing this (useless) step.

Member

GuillaumeGomez commented May 9, 2018

If it's just a wrapper above the C utils, there is no point to bind it since Rust has its own methods to interact with environment. I think you took the good decision by removing this (useless) step.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 9, 2018

Member

Yes, remove

Member

sdroege commented May 9, 2018

Yes, remove

@EPashkin EPashkin changed the title from WIP: OsString support to OsString support and regen May 9, 2018

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 9, 2018

Member

Updated

Member

EPashkin commented May 9, 2018

Updated

@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko May 9, 2018

Contributor

Is there any reason for having functions designed to work with environment,
g_get_environ, g_listenv, g_unsetenv, g_getenv, etc., exposed in Rust?
Especially that Rust ones are a little bit more thread-safe, i.e.,
they are synchronized internally.

Contributor

tmiasko commented May 9, 2018

Is there any reason for having functions designed to work with environment,
g_get_environ, g_listenv, g_unsetenv, g_getenv, etc., exposed in Rust?
Especially that Rust ones are a little bit more thread-safe, i.e.,
they are synchronized internally.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 9, 2018

Member

No, thqt's why we said that he'd better remove them.

Member

GuillaumeGomez commented May 9, 2018

No, thqt's why we said that he'd better remove them.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 9, 2018

Member

But I not removed it completely, only not working.
Also Gio.AppLaunchContext and ApplicationCommandLine also have these functions,
remove all it too?

Member

EPashkin commented May 9, 2018

But I not removed it completely, only not working.
Also Gio.AppLaunchContext and ApplicationCommandLine also have these functions,
remove all it too?

@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko May 9, 2018

Contributor

Those in Gio.AppLaunchContext and ApplicationCommandLine are useful and without direct Rust replacement.

Contributor

tmiasko commented May 9, 2018

Those in Gio.AppLaunchContext and ApplicationCommandLine are useful and without direct Rust replacement.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 10, 2018

Member

@GuillaumeGomez Travis single failure related to futures error, @sdroege already send PR.

Member

EPashkin commented May 10, 2018

@GuillaumeGomez Travis single failure related to futures error, @sdroege already send PR.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 10, 2018

Member

Let's merge then. Thanks!

Member

GuillaumeGomez commented May 10, 2018

Let's merge then. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit a866366 into gtk-rs:master May 10, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@EPashkin EPashkin deleted the EPashkin:os_string branch May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment