Wrong ODOO_VERSION in bootstrap

[ddejong-therp]

I've noticed that when I use the migration script to set up build directories for a migration, it may go wrong because the build has been set up to use a wrong python version.

After investigating this issue, I've noticed that it would set up certain files (like .env-default, .python-version) from the wrong template file.
See these logs for example:

INFO: Link /home/ubuntu/odoo/migration/build-12.0/waftlib/templates/16.0/.env-default to /home/ubuntu/odoo/migration/build-12.0/.env-default
INFO: Link /home/ubuntu/odoo/migration/build-12.0/waftlib/templates/12.0/.python-version to /home/ubuntu/odoo/migration/build-12.0/.python-version

It links .env-default from the 16.0 instead of 12.0
It could link the .python-version from 16.0 instead 12.0 too, which will break a lot more.

See this line: https://github.com/sunflowerit/waftlib/blob/master/bootstrap#L41C66-L41C78
At this point, $ODOO_VERSION is still not necessarily set by anything in waft.
The .env-default file may have been edited to not define the variable, so then it would use the value for $ODOO_VERSION, which could have been previously defined.
In the migration script, $ODOO_VERSION is already defined because waftlib/bin/migrate.py loads them from the .env-* files in the top build directory. Then that script invokes the bootstrap script of another waft build while $ODOO_VERSION is already defined.

I think what needs to happen, is that somewhere at the top of the script the .env-* files need to be called if they exist, so that all the variables (including $ODOO_VERSION) of the waft sub-build are available.
Even if the migration script is not involved, someone could edit and remove the $ODOO_VERSION variable from .env-default, then use ./bootstrap, and then things would break, because $ODOO_VERSION would not be set yet.

[Hussam-Suleiman]

@ddejong-therp I don't see a bug in this part of the code https://github.com/sunflowerit/waftlib/blob/master/bootstrap#L41C66-L41C78 , if ODOO_VERSION is already set before running bootstrab, then bootstrap will not change it because, for example, the ODOO_VERSION variable in ${ODOO_WORK_DIR}/waftlib/templates/11.0/.env-default has a value of 11.0.

Additionally, if you follow a method like the one https://github.com/sunflowerit/waftlib/blob/master/bin/addons#L24-L28 , you will import the variables from .env-default, .env-shared, and .env-secret in sequence. Therefore, .env-default will always hold the lowest precedence for variable values.

@thomaspaulb what do you think?

[thomaspaulb]

@ddejong-therp

The way I've known waft thus far is that upon initial clone for a new project, you can't go wrong because it will warn you to choose an ODOO_VERSION in .env-secret, and then I choose one, and it's OK:

zzz:~/Code$ git clone [email protected]:sunflowerit/waft.git
Cloning into 'waft'...
zzz:~/Code$ cd waft/
zzz:~/Code/waft$ ./bootstrap 
(...)
ERROR: You should define 'ODOO_VERSION' variable in /home/antiflu/Code/waft/.env-secret.

Another usecase is the cloning of an existing project's waft. But, in that case a symlink to the correct .env-default has already been created, so it will clone that too, and bootstrap will clone waftlib first and after doing so it will load .env-default, so it will also always pick the right ODOO_VERSION.

Where it goes wrong is when a certain Odoo version has been set up already, and then I suddenly want a different version, and some old files are still left. In such a case, I have to clean everything up first:

rm -Rf .venv  # remove probably wrong venv
rm -Rf .pyenv  # remove probably wrong Python version
git clean -fd # this won't work if eg .env-default was already committed, but in my usual cases, it's not yet, and then this resets the existing symlinks
rm -Rf custom/src/some/repos  # if a repo is not in repos.yaml anymore but the folder still exists, there's a bug where it still tries to compile that using the newly downloaded, incompatible Python version
./bootstrap  # start over

Because this is most of the time still faster than to re-clone the Odoo and OCA repositories from scratch, I still jump through the above hoops instead of removing everything and cloning a fresh sunflowerit/waft.

As for your migration case, there is probably a way in which it can go wrong, but can't you solve that in the migration script itself? If the ODOO_VERSION variable of the main project is loaded beforehand, that's wrong anyway, so maybe:

• Before you call any sub waft's bootstrap, you can unset all ODOO_* variables, or at least ODOO_VERSION
• As a first init of a sub waft, you can check if its .env-secret is there, if not, you can initialize it with ODOO_VERSION
• You can move the bootstrapping of the main waft's .env below the bootstrapping/building of the sub-waft's
• ...

As to your suggested solution to load all the env variables at the beginning of the waft, I think that does already happen, rather the problem is that it also takes any pre-set variables along for the ride, which could be wrong. So maybe in that case a reset function should be called at the beginning:

https://stackoverflow.com/questions/9671027/sanitize-environment-with-command-or-bash-script

[thomaspaulb]

Finally, a solution could maybe be that we always unset ODOO_VERSION at the beginning of bootstrap and build, and we then load the env variables, and then we check if it's now defined, and if not, we exit.

That way, at least it always fails when it's not defined. And it can't secretly inherit ODOO_VERSION from the caller script.

[thomaspaulb]

@Hussam-Suleiman @ddejong-therp Which do you like best

[Hussam-Suleiman]

In my opinion, we need to improve the script so that it only modifies the variables we expect to be changed during execution.
Maybe something like https://github.com/sunflowerit/waftlib/blob/master/waftlib/__init__.py#L11C1-L50C50

[ddejong-therp]

@thomaspaulb Yeah ok, I think my problems with the migration builds were not really caused by what I thought it was initially. I now understand how to prevent this.

But I still believe it is a (small) bug, that after you've bootstrapped with a different version, and are trying to do it again with another version, it will copy the wrong .env-default template, if you didn't edit .env-shared as well. The templates of .env-shared & .env-default have a line which set ODOO_VERSION, while .env-secret does not.
And because .env-shared is already in place, but sets a different ODOO_VERSION value, it looks weird because you might not have realized that that file is taking precedence over .env-secret in that line I linked.

It probably won't do much harm to be honest, it's just a small nuisance that's all.

I kind of like the solution of unsetting it, and then loading the .env files, like you suggested Tom.

[Hussam-Suleiman]

@ddejong-therp Maybe using sed command to change 'ODOO_VERSION' value in '.env-secret' will be a solution. What do you think?

[ddejong-therp]

Well for the context of the migration script, ODOO_VERSION is already correctly in .env-secret @Hussam-Suleiman .
The problem is that due to the circumstances, ODOO_VERSION also ends up being defined in .env-shared to the externally defined value.

But for the migration script I'm thinking of copying .env-secret .env-shared & .env-default myself, instead of relying on bootstrap to do it for me, and I think it will work fine then as well.

[Hussam-Suleiman]

@ddejong-therp I think, copying .env-secret will be enough, because the variables in .env-secret will override all variables in .env-default and .env-shared

[thomaspaulb]

@ddejong-therp I saw you're now not running bootstrap but copying the files yourself instead.

I'm not sure if you're now still running bootstrap somewhere, if yes, then great, if not, then that's risky I guess because then someone needs to run it manually? I think the correct solution is to copy .env-secret, use sed or any other tool to edit it in place, and then run bootstrap; then, the version should be always correctly defined even on manual rebootstrapping or rebuilding outside of the migration logic.

[ddejong-therp]

@thomaspaulb Yes I'm still running bootstrap somewhere.

It was just a simple workaround around the bug, that I can do to at least continue using the rebuild switch on the migration script.

The correct solution you proposed is essentially what the migration script already does at the moment. The only difference is that instead of sed the migration script has its own function, because it does some extra stuff as well.