JeffGarland / liaw2019-process

Repository for initial drafting of boost.process standards paper
MIT License
5 stars 3 forks source link

this_process::environment #8

Open klemens-morgenstern opened 5 years ago

klemens-morgenstern commented 5 years ago

In boost.process as well as in P1276R0 the environment of the current object is an object. I am not sure this is the right model, since the functions do actually manipulate a global value of the process. I think we could also design those as global functions in a namespace, like so:

std::this_process::environment::get("foo") -> string;//get an environment variable
std::this_process::environment::set("foo", "bar"); //set one
std::this_process::environment::erase("foo"); //delete it

//Get an object with the actual handle of the global object. In windows this has to be created & delete by a function; on posix it's a global char**
std::this_process::environment::raw() -> (char**  for posix | smart_ptr with customer deleter on windows);

std::this_process::environment::path() -> std::vector<std::filesystem::path>; 
std::this_process::environment::shell() -> std::filesystem; 
//Search through the path -> on windows, take extensions into account.
std::this_process::environment::find_executable(std::string & name) -> std::filesystem::path;

//Construct a copy we use in the child process.
std::environment env = std::this_process::environment::raw();

Thoughts?

klemens-morgenstern commented 5 years ago

Note @self: the shell ought to depend on the environment:

FatihBAKIR commented 5 years ago

Having the functions to modify the environment of the current process as a namespace would prevent me from writing a generic function that can modify both the environment of a to-be-launched process and mine like this:

template <class EnvironT>
void set_some_env_var(EnvironT&& env)
{
    env.set("SOME_KEY", some_val());
}

set_some_env_var(std::this_process::environment());
...
std::environment new_env = std::this_process::environment();
set_some_env_var(new_env);

I think having consistency between the environment of this process and other processes would be nice.

JeffGarland commented 5 years ago

interesting point. damn generic code is always messing with us ;)

I'm feeling that the env stuff is going to wind up being a big topic of discussion since in the final analysis we're really saying something a bit different is needed than the current proposal.

So you might want to have a look at this section of the paper which probably make portable generic code even more difficult

*** operator[]

Unlike Muerte's proposal (P1275), this proposal does not contain an operator[]. The reason is that environment variables are not uniform on their handling of case-sensitivity. For example "PATH" might be "Path" between different versions of Windows. However, both maybe defined on Windows. This can cause a problem:

+BEGIN_SRC c++

std::environment env = std::this_process::environment::native_environment();

// Let's say it's "Path", but we expect "PATH" env["PATH"].add_value("C:\python"); std::process proc (env.find_executable("python"), {"./my_script.py"}, env); // Error: python not found or ambiguity error.

+END_SRC

Thus this proposal makes the ambiguity explicit.

eliaskosunen commented 5 years ago

Just to remind everyone, the next revision of P1275 is going to have a mutable std::environment, and a global std::env instance of that representing the current environment (unless Isabella changed their mind mid-week)

JeffGarland commented 5 years ago

It would be a 'reminder' if I knew that, but I did not. Clearly more discussion with Isabella is needed. I realize you've been corresponding, have you sent her info on what we're up to?

eliaskosunen commented 5 years ago

It was in the bottom of the first post of #15

klemens-morgenstern commented 5 years ago

@eliaskosunen Sounds good to me. So what we have as std::this_thread::environment would become std::env and std::environment just needs the ProcessInitializer hooks. Works for me. I consider shell, path and find_executable quite important, mainly to avoid boilerplate code for users. BUT it is not essential, even though my guess is a lot of people will reimplement it.

@eliaskosunen Maybe a transaction like version would be even better, i.e.:

std::environment my_env = std::this_process::native_environment(); //or get_env
//apply your logic
std::this_process::set_env(my_env);

I think seperating the usage of the two is a good idea, since doing several changes on the current environment can have side effects, where one write might be superior. Or at least the semantics are quire different.

JeffGarland commented 5 years ago

I still can't find the text that in #15 that says it will be modifiable. But really non of this matters in the short term -- I think we go with what is there, and sort it out over time. If it goes well we will just influence 1275 to have what we need.

klemens-morgenstern commented 4 years ago

Has there been any news on that?