Page MenuHomePhabricator

ecore: here comes a env object
Needs ReviewPublic

Authored by bu5hm4n on Tue, Dec 25, 9:10 AM.

Details

Summary

the env object can be used to alter and edit the content of environment
variables. Additionally, the class efl.core.env can be used to to setup
a not applied set of environment variables, which then can be applied
later (in the future) to set it directly to a spawned process for
example, or as a general key/data storage. A efl.core.env object can
also be forked off, which makes it easy to customize predefined objects.

ref T7514

Depends on D7509

Diff Detail

Repository
rEFL core/efl
Branch
devs/bu5hm4n/submit
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8664
bu5hm4n created this revision.Tue, Dec 25, 9:10 AM
bu5hm4n requested review of this revision.Tue, Dec 25, 9:10 AM
bu5hm4n updated this revision to Diff 18101.Wed, Dec 26, 2:29 AM
bu5hm4n edited the summary of this revision. (Show Details)

a better test

100% coverage for efl_core_env

~70% efl_core_proc_env

bu5hm4n updated this revision to Diff 18111.Wed, Dec 26, 9:09 AM

new api.

zmike requested changes to this revision.Wed, Dec 26, 10:50 AM

This should have a travis build success submitted prior to landing since it modifies build files.

src/lib/ecore/efl_core_env.c
26

This should probably also check for null strings like "".

34

This should probably also check for null strings like "".

40

You could just free the hash buckets?

74

I'm not sure I like the expectation of casting here. Do we want to have a generic "Efl_Tuple" data type for things like this?

src/lib/ecore/efl_core_env.eo
2

nit: space before brace.

17

It would be great if this could have both an example string as well as a method for obtaining it. Also based on this description alone it's unclear to me whether this is supposed to be the entire env string (ie. output of printenv) or a single key:value pair.

34

nit: speling

40

Maybe better to reword as "Equivalent to calling unset on all contained environment variables" or some variation on that?

42

Should this use efl.duplicate?

50

nit: tuple

src/tests/ecore/efl_app_test_env.c
2

This should maybe have some kind of destructor test to verify that the destructor is functional?

69

I think technically ck_assert(env); would also be fine here; The ptr_nonnull test may require a newer version of check than we are able to use.

This revision now requires changes to proceed.Wed, Dec 26, 10:50 AM

What advantages does this new class have over a simple Eina_Hash?
The implementation of Efl.Core.Env seems just a thin wrapper over Eina_Hash, so why not make Efl.Core.Proc_Env accept an Eina_Hash directly?

Also, no autotools support?

What advantages does this new class have over a simple Eina_Hash?
The implementation of Efl.Core.Env seems just a thin wrapper over Eina_Hash, so why not make Efl.Core.Proc_Env accept an Eina_Hash directly?

Also, no autotools support?

Autotools support is indeed missing everywhere in this series.

The reason for Efl.Core.Env to exist is basically to have a described API (Which API call is doing what ? You cannot attach that information to a set call of a hash, and documenting it somewhere else does not make it showing up in a IDE), additionally, there is no fork method in Eina_Hash which would also create the question of ownership, (Who owns the strings and keys etc. now?) Further more, in C handling eina_hash by hand is a little bit error prone caused by the fact that you need to pass things with & to the find method, so addresses are passed that are later referenced again, this is _not_ obvious or easy to catch.
Also, considering the workflow that you would have with a hash: Give me a hash that represents the environ, edit a line, tell *some* object that a line has changed, (feels a bit fragile).
Additionally, i like to keep away internal datatypes as far away from evertything as I can, so later changes to the object (which might change its internal) are not affecting the API, (Thus just APIs, + a iterator for getting the content)
:)

zmike added a comment.Thu, Dec 27, 9:53 AM

Using hash directly seems to me like a potentially bad idea in many cases? If you consider the case of bindings (don't higher level languages have better facilities for things like this anyway though?), they'll have their own key:value data struct implementation and it will feel pretty bad if we force them to use eina.

In D7510#131807, @zmike wrote:

Using hash directly seems to me like a potentially bad idea in many cases? If you consider the case of bindings (don't higher level languages have better facilities for things like this anyway though?), they'll have their own key:value data struct implementation and it will feel pretty bad if we force them to use eina.

The expectation is that any eina type in a .eo will have a manual binding that should allow for seamless usage of the native language type. So when you would have an Eina_Hash in the C API generated by Eo, you would see a native JS Hash for example in JS. At least that's the point of hidding the type in the .eo and just referring to list<> for example.

Yeah, as @cedric says, the idea in my head was that we use Eina_Hash in C land and whatever dictionary structure each language provides.
However, you all seem to have thought about this far more than I have done so I won't push it any further :)

raster requested changes to this revision.Thu, Dec 27, 10:26 AM
raster added a subscriber: raster.

eina hash just can't work well. please see my comments on D7514 - you end up with the same "it works differently in different places", a less efficient implementation as you have to scan the hash and the environment every time you set it (after modifications) to sync it etc. etc. but eina_hash is worse because now you don't have any ownership of the strings. do you have to stringshare_add them when adding to hash? or just cosnt char *'s? if the latter then we have all the nasty problems putenv() does (see manual pages - but specifically that the string ptr is used AS-IS int he env so you cannot release the string you pass in - it's not duplicated and so tracking this string's lifetime and ensuring it's duplicated and kept alive as long as in use etc. etc. also nukes the idea of an eina hash).

but this whole env object is tied to D7514 too so imho this is not better, it's worse. it's more code and complexity to use, more places for sync issues, no actual improvement over what is there (differing behavior on exe vs loops (tasks)), more possibilities for things to go wrong etc. - see D7514

bu5hm4n updated this revision to Diff 18166.Tue, Jan 1, 3:58 AM

update to comments

@zmike comments are reworked.

@raster:

  • The biggest part of this code is copied from efl_app & efl_task, so i am not too sure where it is really more complexity, the only difference is that you have a template object, and a place where you apply it. thats all.
  • To the point of the actual improvement: You have a smaller object, which can be easier tested, (which actaully IS tested). This is a way smaller logical unit than the whole efl_task thing, and thus easier to learn, since you don't have to deal with the whole size of efl_task APIs but rather only efl_core_env.

Also from the other thread:

  • The previous implementation does not support something like fork(obj) which has proven to be super usefull for something like exe. (See the python API).
  • Its not really obvious to see that the actual env API of efl_task does not _mirror_ to the environment, as it is in the efl_thread object.
  • The documentation changes you proposed are not possible as no language supports those documentation changes on inheritance.
src/lib/ecore/efl_core_env.c
74

A *generic* data type for tuples is going to be a pain in bindings, caused by the fact that we don't have generics, to have a generic tuple we would need to have void_ptr or something as key and data structure, which is a pain to use in bindings, thus i decided to have a efl_Core_Env_Tuple, which is just a string, string tuple. This also gives the possibility to explain the ownership there.

src/lib/ecore/efl_core_env.eo
42

Mhmmm the problem with Efl.Duplicate is that it is not really usefull in a binded language.

In C# we would have to upcast the returned value of that call to Efl.Core.Env to have a env object again. Which makes it a little hard, when this is solved, then i see no harm in using it ... :)

bu5hm4n marked 9 inline comments as done.Tue, Jan 1, 4:03 AM
cedric added a comment.Wed, Jan 2, 9:47 AM

but eina_hash is worse because now you don't have any ownership of the strings. do you have to stringshare_add them when adding to hash? or just cosnt char *'s?

For all native type we specify the ownership explicitely, otherwise we could not have any list<>, iterator<> or whatsoever container. This is core to every binding needs, clear definition of pointer ownership. So this is an API decision to be made, but should not be a constrain from the .eo. If it is a constrain from the .eo, that need to be fixed independently.

bu5hm4n updated this revision to Diff 18219.Fri, Jan 4, 3:25 AM

buildsystem update

raster added a comment.Fri, Jan 4, 3:48 AM

The biggest part of this code is copied from efl_app & efl_task, so i am not too sure where it is really more complexity

this is more complexity because someone to change the env has to create an env object then manipulate it THEN attach it to the ytarget as opposed to just call existing methods on the target to manipulate it. you are making something worse, not better.

You have a smaller object, which can be easier tested, (which actaully IS tested)

testing the object in isolation is not sufficient. it has to be tested in it's use cases. as a singleton global AND as an obj you pass into exe's and check the children get the resulting env. it needs up being the same amount of testing in the end. it's not less. users of the api have to write more lines of code (as above) and the rest is the same, except for bindings. a singleton global now has to be exposed to all bindings by hand unless you make it a method on the task class to get the env obj and then it's still extra code to get it, modify it and set it back. if you put it in the app class and the exte class separately to have one be get only (app) and one be set+get (exe) and then one it appthread (get only) you make it even more complicated as it's a different method name in c per class thus reducing re-usability of knowledge. if you keep it in task then a set has no effect on the app object and this needs to be tested too for misuse that someone creates an env obj and then sets it to the app - if this is not a shared obj we're in trouble.it's easier to shoot yourself in the foot this way in this design.

This is a way smaller logical unit than the whole efl_task thing, and thus easier to learn, since you don't have to deal with the whole size of efl_task APIs but rather only efl_core_env.

also it's not easier or harder to learn. if you think everyone has to learn every single method in a class to know how to use it then you are right, but this i think is utterly false. you look for the methods you need when you need them. if your logic is you must learn every method then you have to learn every object class type that every method can accept and all its methods too thus ending up with the same amount of learning. if you are not interested in messing with the environment you just don't have to look into the env methods. like any other method in any class. as a set of methods on a class you learn it once if you ever need it and use it in the same way everywhere. with your way you have to know to create env objects for exe's but to get them for thread/app objects (and not create). it's more learning your way.

The previous implementation does not support something like fork(obj) which has proven to be super usefull for something like exe. (See the python API).

we should absolutely not implement anything like fork(). it's a black hole of pain. fd's are shared (e.g. pipes) and you have to always carefully disentangle these. in fact all our use of fork in e and efl has been nothing but pain and i have steadily removed forks with running separate exe's and using args/stdio because of this. this is NOT something to aspire to. it's to be avoided. not to mention OS's like windows don;'t have fork. it, at best, is emulated in an insanely slow way and we're best off just avoiding this like the plague.

Its not really obvious to see that the actual env API of efl_task does not _mirror_ to the environment, as it is in the efl_thread object.

what do you mean exactly?

The documentation changes you proposed are not possible as no language supports those documentation changes on inheritance.

Perfectly possible - put it in the class generic section indicating that environment for app and appthread are shared across the process as a whole and that changes are visible to both sides and in the exe class document that the environment is immediate inherited from the parent process (which is what fork+exec(), system() etc. all do anyway by default so i never saw a need to document that, but if it is not obvious then indeed it should be documented). :)

The biggest part of this code is copied from efl_app & efl_task, so i am not too sure where it is really more complexity

this is more complexity because someone to change the env has to create an env object then manipulate it THEN attach it to the ytarget as opposed to just call existing methods on the target to manipulate it. you are making something worse, not better.

Yeah 2 more calls for attaching a object and creating it. That is true, however, you have the possibility to have the same environment that you can attach to multiple execs running in parallel for example. which allows a better seperation. (This argument can continue with *this is better because you can do seperation of concerns etc. etc. see our discussion in D7514)

You have a smaller object, which can be easier tested, (which actaully IS tested)

testing the object in isolation is not sufficient. it has to be tested in it's use cases. as a singleton global AND as an obj you pass into exe's and check the children get the resulting env. it needs up being the same amount of testing in the end. it's not less. users of the api have to write more lines of code (as above) and the rest is the same, except for bindings. a singleton global now has to be exposed to all bindings by hand unless you make it a method on the task class to get the env obj and then it's still extra code to get it, modify it and set it back. if you put it in the app class and the exte class separately to have one be get only (app) and one be set+get (exe) and then one it appthread (get only) you make it even more complicated as it's a different method name in c per class thus reducing re-usability of knowledge. if you keep it in task then a set has no effect on the app object and this needs to be tested too for misuse that someone creates an env obj and then sets it to the app - if this is not a shared obj we're in trouble.it's easier to shoot yourself in the foot this way in this design.

With leaving out the same argument that you have written above: The testing of the obejct itself ensures that the storage is working as intended, then you just have a single call to to set the object and now you simply check if the state that you have setted on the object is applied to the (as an example) executed process. All in all this is easier and less testing (n-APIs on 1 object + 1 set API vs. 2x n-APIS on 2 objects). So it is easier.
For the part of the appthread, please see my reply in D7514 i already said why this is not that usefull on a thread and should probebly be a storage object with more relaxed rules, or probebly be completly left ot the user otherwise.

This is a way smaller logical unit than the whole efl_task thing, and thus easier to learn, since you don't have to deal with the whole size of efl_task APIs but rather only efl_core_env.

also it's not easier or harder to learn. if you think everyone has to learn every single method in a class to know how to use it then you are right, but this i think is utterly false. you look for the methods you need when you need them. if your logic is you must learn every method then you have to learn every object class type that every method can accept and all its methods too thus ending up with the same amount of learning. if you are not interested in messing with the environment you just don't have to look into the env methods. like any other method in any class. as a set of methods on a class you learn it once if you ever need it and use it in the same way everywhere. with your way you have to know to create env objects for exe's but to get them for thread/app objects (and not create). it's more learning your way.

I was more referring to this: You can with the same API cause 3 different things to happen, which are depending on the MOMENT of the execution (relative to the object state) to the object you are calling it on. You need to learn those differences, and understand them, and those can easily cause bugs.
With the new approach, you don't have that problem, (as long as you don't pass the singleton arround, which should be avoided anyways, even if it is done, then it is still only 2 different semantics, and not 3). So all in all, this is easier to learn from a object POV. And additionally, efl_task is easier to understand since there is lesser API ...

The previous implementation does not support something like fork(obj) which has proven to be super usefull for something like exe. (See the python API).

we should absolutely not implement anything like fork(). it's a black hole of pain. fd's are shared (e.g. pipes) and you have to always carefully disentangle these. in fact all our use of fork in e and efl has been nothing but pain and i have steadily removed forks with running separate exe's and using args/stdio because of this. this is NOT something to aspire to. it's to be avoided. not to mention OS's like windows don;'t have fork. it, at best, is emulated in an insanely slow way and we're best off just avoiding this like the plague.

Well my comment here was refereing to the object API, which is the fork call in the Efl.Core.Env object...

Its not really obvious to see that the actual env API of efl_task does not _mirror_ to the environment, as it is in the efl_thread object.

what do you mean exactly?

app.env_set("BLA","FOO"); //changes you environment
thread.env_set("BLA","FOO"); //changing something for the thread
exe.env_set("BLA","FOO");//will be applied later to the process

All of this, is not documented in a single place. and additionally, it cannot be documented, as the documentation of a API cannot be changing based on inheritance in most languages

Proc_Env.set("BLA","FOO") //changes you environment
^ Clear place to document that this object is mirrored to the enviroment of the process, which is also indicated by the name.
tmp = new ...
tmp.set("BLA","FOO");
exe.set_environment(tmp);//will be applied later to the process
^ Clear place to ducument that the API set_enviroment is going to apply the template object to the execution

The documentation changes you proposed are not possible as no language supports those documentation changes on inheritance.

Perfectly possible - put it in the class generic section indicating that environment for app and appthread are shared across the process as a whole and that changes are visible to both sides and in the exe class document that the environment is immediate inherited from the parent process (which is what fork+exec(), system() etc. all do anyway by default so i never saw a need to document that, but if it is not obvious then indeed it should be documented). :)

#1 I am not talking about that the env vars are inherited from the main process, i am talking about that the env* APIs are not documented to only be used BEFORE the run method is called, and that they alter and change the *later-applied* enviroment.

#2 You can write this documentation into the class header, this is true, you can also write them into the tutorial or so. However, the point of the documentation here is that they should be documented in the place where the user needs them and sees them. Thus the documentation in the set api of Efl.Core.Env documents that this is setting the fields to this objects internals, later you call set_environment on the exec object which documents that this environment instance will be applied to the execution once run is called, calling this later is erroring (etc. etc.). The same argument for Efl.Core.Proc_Env is that the Api to get the object there is documented to do that, its not precisely on point but as close as possible, using Efl.Core; [...] Proc_Env.environment.set would be the same there, the Proc_Env is documented to mirror changes to the real enviroment, not perfectly on the function, but very close.