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

Add Windows implementations for g_setenv, g_getenv, g_unsetenv and g_get_current_dir #171

Merged
merged 3 commits into from May 21, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented May 21, 2017

There are equivalent functions with the _utf8() prefix on Windows, to which macros with the "normal" names are pointing.

See #169 (comment) and below

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

@EPashkin Would probably make sense to merge this before #169 , or do #169 only with the new BoolError. And then afterwards setenv() can be changed to return a BoolError in another pull request. Too many interdependencies.

Member

sdroege commented May 21, 2017

@EPashkin Would probably make sense to merge this before #169 , or do #169 only with the new BoolError. And then afterwards setenv() can be changed to return a BoolError in another pull request. Too many interdependencies.

Show outdated Hide outdated src/utils.rs
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

@sdroege I don't like define ff-function in normal way, and prefer doing it later with gir starting with sys crate.
Thanks for pointing error in get_current_dir I missed this when checked generated code (also it better return Option<std::path::PathBuf>)

@GuillaumeGomez What you think?

Member

EPashkin commented May 21, 2017

@sdroege I don't like define ff-function in normal way, and prefer doing it later with gir starting with sys crate.
Thanks for pointing error in get_current_dir I missed this when checked generated code (also it better return Option<std::path::PathBuf>)

@GuillaumeGomez What you think?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

don't like define ff-function in normal way, and prefer doing it later with gir starting with sys crate.

The problem is that you won't have it in the .gir file, unless you generate it on Windows. And then you won't have the non-UTF8 one. That's why I thought declaring it here would be best.

Member

sdroege commented May 21, 2017

don't like define ff-function in normal way, and prefer doing it later with gir starting with sys crate.

The problem is that you won't have it in the .gir file, unless you generate it on Windows. And then you won't have the non-UTF8 one. That's why I thought declaring it here would be best.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

As we don't have separate gir files for windows (using ubuntus),
we only can add for Gir's config parameter that add these function in sys mode,
and use it in normal mode.
But your can stays if GG agreed with it.

Member

EPashkin commented May 21, 2017

As we don't have separate gir files for windows (using ubuntus),
we only can add for Gir's config parameter that add these function in sys mode,
and use it in normal mode.
But your can stays if GG agreed with it.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

Checked that getenv work on Windows with local (Cyrillic) characters

Member

EPashkin commented May 21, 2017

Checked that getenv work on Windows with local (Cyrillic) characters

sdroege added some commits May 21, 2017

Add Windows implementations for g_setenv, g_getenv, g_unsetenv and g_…
…get_current_dir

There are equivalent functions with the _utf8() prefix on Windows, to
which macros with the "normal" names are pointing.
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 21, 2017

Member

It's a bit strange to have these C functions declared directly into the rust binding. However, it seems quite difficult to be able to do it otherwise (if it's not provided by the gir file, we have to do it by hand). So for me it's ok, I'm just a bit disappointed and hope that this won't be too much of a burden to maintain...

Member

GuillaumeGomez commented May 21, 2017

It's a bit strange to have these C functions declared directly into the rust binding. However, it seems quite difficult to be able to do it otherwise (if it's not provided by the gir file, we have to do it by hand). So for me it's ok, I'm just a bit disappointed and hope that this won't be too much of a burden to maintain...

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

if it's not provided by the gir file

Especially in GLib >= 2.53 it will not even be in the headers anymore, only provided for backwards compat as a symbol for the next few years.

I'm just a bit disappointed and hope that this won't be too much of a burden to maintain

That API is not going to change in the next years, and the commit @EPashkin linked on the other pull request actually cleans up this mess... so in a few years we can have everything nice and good :)

Member

sdroege commented May 21, 2017

if it's not provided by the gir file

Especially in GLib >= 2.53 it will not even be in the headers anymore, only provided for backwards compat as a symbol for the next few years.

I'm just a bit disappointed and hope that this won't be too much of a burden to maintain

That API is not going to change in the next years, and the commit @EPashkin linked on the other pull request actually cleans up this mess... so in a few years we can have everything nice and good :)

@GuillaumeGomez GuillaumeGomez merged commit 0302b5c into gtk-rs:master May 21, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment