[Buildroot] [PATCH v3 2/9] testing/infra/builder: split configure() from build()

Ricardo Martincoski ricardo.martincoski at gmail.com
Sun Apr 1 21:32:01 UTC 2018


Hello,

On Sun, Apr 01, 2018 at 02:59 PM, Thomas Petazzoni wrote:

> On Sun, 29 Oct 2017 12:06:01 -0200, Ricardo Martincoski wrote:
> I've applied, but I have a comment, see below.
> 
> 
>> +    def build(self):
>> +        env = {"PATH": os.environ["PATH"]}
> 
> So now, we have the same local "env" variable in configure() and
> build(). Would it make sense to do:
> 
> 	self.env = ...
> 
> in the constructor, and then use it in configure() and build() ?

It is possible to do that.

The next patch on the same series uses .update() so the local "env" is changed.
        env = {"PATH": os.environ["PATH"]}
        env.update(make_extra_env)

Note: I did not tested the code below.

In order to always have a clean environment, we could do in the constructor:
        self.env = {"PATH": os.environ["PATH"]}
And change the methods to:
        env = self.env.copy()
        env.update(make_extra_env)

Or we assume we will need only PATH from env in the future, and do in the
constructor:
        self.env_path = os.environ["PATH"]
And change the methods to:
        env = self.make_extra_env.copy()
        env.update({"PATH": self.env_path})

The access to os.environ should be inexpensive (I did not measured it), so I am
not sure it needs to be done. I am not against it either.
At first glance, it seems just more code to me.
Maybe I am missing something.


Regards,
Ricardo


More information about the buildroot mailing list