BorisSchaeling / boost-process

Boost.Process is a library to manage system processes
Boost Software License 1.0
32 stars 20 forks source link

Removed usage of Windows.h #7

Closed klemens-morgenstern closed 8 years ago

klemens-morgenstern commented 8 years ago

So I want to use the library and bloody hate the win-api, hence i removed the usage of it and build in substitutes.

The thing compiles with MSVC 2015 and Mingw 5.1 but not all tests pass. The problem here is, that I do not know the library well enough to debug this. Additionally, I'd need to know if you'd be even interested in this change, since I'd guess we'd also have to provide tests for the winapi-stuff or may have more work to comply to the boost conventions.

So if you've got the time, I'd be really glad if you could look over the code.

Hopefuly this closes #4.

klemens-morgenstern commented 8 years ago

I will try to move the changes to boostorg/winapi, and as soon as this is accepted rework this pull request to make more use of this. However, as an experimental implementation it's quite useful already.

BorisSchaeling commented 8 years ago

Thanks for your patch! I agree with you that boostorg/winapi is a better place though. Current and future Boost.Process maintainers can then concentrate on process management only (which is complicated enough :) without having to dive into additional details of the Windows API.

klemens-morgenstern commented 8 years ago

Currently the pull request is pending. If that is accepted we could pull - that might however need to be on the develop branch, since we need the develop branch of develop

klemens-morgenstern commented 8 years ago

Ok, it's officially broken, I'll work on that, and tell you as soon as fixed.

BorisSchaeling commented 8 years ago

Sounds good! - Do I understand correctly that you recommend us creating a develop branch?

klemens-morgenstern commented 8 years ago

@BorisSchaeling Currently, windows provides two versions for the functions, which may either take wchar_t or char_t. Now I changed boost::filesystem::path to be converted to wchar_t by default, because wchar_t is the default representation on windows. Since the executor also uses the wchar_t types currently (aea733727443b2eb77aa563f03352ce926822a63) , the tests fail.

I would propose the following:

Make executor into a template, and defined typedefs in the following way:

template<typename CharType> struct basic_executor;

typedef basic_executor<char>         executor;
typedef basic_executor<wchar_t> wexecutor;

The other variant would be to just make everything use wchar_t and just convert everything into wchar_t.

Edit: I forgot to mention: I changed the underlying classes to be available if they are available on the platorm. From what I can tell, the WCHAR version is always available, while the CHAR might not be (which afaik is a rather obscure case).

I would then also look for the parameters in execute, whether one of them needs wchar_t, and thereby select if it'll use executor or wexecutor. If this double implementation is not needed, I think I'd use the WCHAR variant, as boost::filesystem::path does.

klemens-morgenstern commented 8 years ago

Well, I am just saying: until the winapi changes go into the master branch will take a while. It would be strange to have process/master dependent on winapi/develop.

But that's not relevant for travis, since it only builds the linux version.

klemens-morgenstern commented 8 years ago

Ok, this branch could actually be merged, if the user considers, that boost::filesystem::pathwill be converted to an std::wstring. I am rather hesitant to implement an auto-conversion, since this would require me to rework the execute function,

klemens-morgenstern commented 8 years ago

I would consider this PR obsolete since the development of the new version of boost.process seems to lead somewhere