Re: [Fuego] [PATCH 2/4] linaro: update to python3

> -----Original Message-----
> From: [email protected] <[email protected]>
> 
> Hi Tim,
> 
> Thanks for your review.
> See my comments inline.
> 
> > -----Original Message-----
> > From: [email protected] <[email protected]>
> > Sent: Friday, August 27, 2021 2:54 AM
> > To: sangorrin daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ) <[email protected]>
> > Cc: [email protected]; nguyen dat tho(ＴＳＤＶ Ｅｎｇ　１) <[email protected]>
> > Subject: RE: [PATCH 2/4] linaro: update to python3
> >
> > Ok - I went to apply this today, and I realized I still have some questions...
> >
> > See inline below.
> >
> >
> > > -----Original Message-----
> > > From: Daniel Sangorrin <[email protected]>
> > >
> > > From: Nguyen Dat Tho <[email protected]>
> > >
> > > The upstream linaro repository[1] now uses python3 so use pip3 to
> > > install the requirements
> > >
> > > [1] https://github.com/Linaro/test-definitions
> > >
> > > Signed-off-by: Nguyen Dat Tho <[email protected]>
> > > Signed-off-by: Daniel Sangorrin <[email protected]>
> > > ---
> > >  tests/Functional.linaro/fuego_test.sh | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tests/Functional.linaro/fuego_test.sh
> > > b/tests/Functional.linaro/fuego_test.sh
> > > index 677ab49..c775baa 100755
> > > --- a/tests/Functional.linaro/fuego_test.sh
> > > +++ b/tests/Functional.linaro/fuego_test.sh
> > > @@ -26,8 +26,10 @@ function test_pre_check {  }
> > >
> > >  function test_build {
> > > +    apt-get install python3-pip
> > >      source ./automated/bin/setenv.sh
> > > -    pip install -r $REPO_PATH/automated/utils/requirements.txt --user
> > > +    pip3 install setuptools --user
> > > +    pip3 install -r $REPO_PATH/automated/utils/requirements.txt
> > > + --user
> > >  }
> >
> > What is the effective user account when this is run in Fuego non-container mode?
> 
> We normally install fuego as root and without jenkins. So it would be running with root permissions.
> 
> > For non-container execution, do you execute build steps as user 'fuego'
> > or 'jenkins'?
> 
> We use root. However, you can also install jenkins even when you don't use containers.
> 
> https://bitbucket.org/fuegotest/fuego/src/caf37be917f3756980879f32ad044b8f240fbd25/install-native.sh#lines-97
> 
> So in that case, it would jenkins.
> 
> > For a container environment, the build steps are performed as user 'jenkins'.  Will these steps even work, in that
> > environment?
> 
> I believe they should work, but we will double check and let you know.
> 
> > Does this need to be 'sudo apt-get install', to handle the container case?
> 
> Probably we can use the Debian packages, we haven't tested that. Also the requirements.txt from Linaro unfortunately does not specify
> versions.

What is this requirements.txt file?  I'm not well-versed on pip and how it works (other than that it
downloads packages and resolves dependencies, like most package manager).
Does this requirements.txt have the version of python modules that are needed for Linaro's tools?
Does it indicate the list of python modules required for every test in the test-definitions git
repository, or just the python modules needed for the Linaro core itself?

The reason I ask is to get a sense of the scope of the items that will be installed when this
is executed.

where does REPO_PATH come from in this test?  (I'm going to guess it comes from setenv.sh)

> 
> > Do the pip3 operations work at the system level, or at the level of an individual account.
> 
> I think that if you run them from jenkins it will install them using jenkins (although i am not sure what happens if jenkins does not have a
> $HOME folder). We run it with root so i guess they go to /root/.cache/pip or something like that.
> 
> > If a better python3-pip is needed for building, I think this should go into the installation scripts, unless this is intended to be doing
> > something on the device under test, and not the host.
> 
> This was intended to run on the host for host-target configurations, and on the DUT for native configurations (eg. LAVA).
> 
> We can put them on the installation scripts, I also thought about this.
> The only bad thing is that if the upstream repository changes the requirements.txt file, we might not notice.
I agree. That's very likely.
> 
> Also, it is easier to understand the dependencies of each test if we put them there. Actually this should be handled better in general.
Agreed.  I have struggled with how much to put into the docker container (or host system, for native installs) to
support individual tests, and when exactly to put them there.

If you put every possible needed tool in the container, for all tests, you end up with a pretty bloated container.
If you defer loading the needed tools and packages until a test is actually invoked, it keeps unnecessary
bloat out of the container, but it risks items not finding out about dependency issues until then.

I see pros and cons of both approaches. 

> 
> Should I move this to the install scripts then?

See below for my thinking on this.

> 
> In that case, we will install the requirements without --user (or perhaps using Debian packages) .
> Regarding the upstream requirements.txt file, we can ignore until the test doesnt work or we can pin the current commit id.
> 
> > This will have a permanent effect on the build environment, for either the container, or the host machine where Fuego is running.
> 
> Correct.
> 
> > In your test environment, is the host machine the same as the device under test, so that these changes are thrown away when the
> > image is thrown away?  Or is this persistent?
> 
> We want to use the LAVA model of always testing on a pristine image. So after running all tests, the image is discarded.

So this works pretty well when you're doing the LAVA thing, with Fuego native on the device under test.
It gets dicier when this is run in the docker container.  This test_build potentially changes the contents of
the python3 site-packages for the Jenkins user - which will affect on an ongoing basis all the other 
tests on the system.

In a perfect world, we would sandbox this, and create a separate custom site-packages directory
just for the Linaro tests (maybe inside the /fuego-rw/buildzone/Functional.linaro directory).  This avoid
changing the python modules for other tests.

However, having said that, I don't think we have a ton of tests that use python3 modules, or especially
any esoteric python3 modules, which would create incompatibilities.  So setting up a private site-packages
directory per test seems like overkill.  I reserve the right to do so sometime in the future, though, if we
run into conflicts.

> 
> > It seems like there should be a check to avoid re-installing these, if they are already present.  But maybe "apt-get install " or "pip3
> > install" will just issue a warning, and there is not harm to executing these if the packages are already there.
> 
> Yes, they would only issue a warning. But if we remove test_build and move the dependencies to the install scripts, then we will not need
> checks at all.
> 
> >
> > Anyway, as  you can tell, I am confused by this patch.
> >
> > Overall, I don't object to it.  I just want to understand what's happening with pip3 installation and setuptools installation at the user
> > level and system level, for the case where:
> >  - the test is executing in a Fuego container, with a remote DUT
> >  - the test is executing in a Fuego container, with a local DUT (DUT=self)
> >  - the text is executing natively, with a remote DUT
> >  - the test is executing natively, with a local DUT (DUT=self)
> 
> Sorry for the confusion.
> 
> We will move the run dependencies to the install scripts. Then, we will test that those combinations work and what user/location is used to
> install the pip packages.
> 
> What do you think?

OK - pip3 and (python3) setuptools should be up-to-date at the system level.  So I'd like to see the
installation of those put into the install scripts.  We'll be using more and more python3 code in
the future, so I think this would be good.

For the packages specifically needed by Linaro tests definitions, I'm OK with deferring filling out the
modules from requirements.txt into a local (--user) account, to when the Linaro test is run.

So I think I'd like to see this line stay in the test:
 +    pip3 install -r $REPO_PATH/automated/utils/requirements.txt --user

Is that OK?

 -- Tim