Fix dependencies expansion: handle active=True correctly #93

joao-p-marques · 2021-04-15T11:51:18Z

active=True in a module's manifest is a deprecated equivalent to auto_install=True:
odoo/odoo@2cc7f0d#diff-367842087e68d6811cf53f2ab3eb21103cd8e793b1648ef3e57f220ee0ccde4eL345

It does not mean that Odoo installs it automatically, as mentioned in #9

Instead, we should handle that key the same way we handle the auto_install one.

To reproduce

In v13 or v14, there is a localization module with active set to True: https://github.com/odoo/odoo/blob/13.0/addons/l10n_fi/__manifest__.py#L39
If we did click-odoo-initdb -n devel -m base, l10n_fi was added to the list of modules to generate the hash, along with its dependencies (one of them is account). However, after the initialization of the DB, Odoo did not install that module, which made that hash incorrect.
If we, then, removed the DB (click-odoo-dropdb devel) and initialized it again with account (click-odoo-initdb -n devel -m base,account) the result was that a DB cache was restored (because the previous hash included account) when, however, account was never installed in the DB.

@Tecnativa
TT24972

ping @yajo

joao-p-marques · 2021-04-15T12:36:27Z

@sbidoul are the test errors related to this change?
I see that they fail for master too: https://github.com/acsone/click-odoo-contrib/runs/2064361001?check_suite_focus=true

sbidoul · 2021-04-15T13:13:34Z

The test failures are not related. We'll need a new test for this case though.

joao-p-marques · 2021-04-15T13:41:09Z

Ok, I added a simple test with this situation in particular, I don't know if it is the best way of testing this, but it tests at least this specific situation.

sbidoul · 2021-04-21T21:46:33Z

@joao-p-marques thanks for the fix! Can you rebase and add a newfragments/93.bugfix with a changelog entry ?

I don't know why we decided to handle active differently than auto_install in #9. That's weird.

joao-p-marques · 2021-04-22T05:51:10Z

Ok @sbidoul Thanks!

I just added newfragments/93.bugfix to my commit. Do I need to do anything else to update the changelog?

codecov-commenter · 2021-04-22T06:00:40Z

Codecov Report

Merging #93 (01f6d5e) into master (478983c) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
- Coverage   93.16%   93.12%   -0.04%     
==========================================
  Files          14       14              
  Lines         892      887       -5     
  Branches      157      154       -3     
==========================================
- Hits          831      826       -5     
  Misses         43       43              
  Partials       18       18

Impacted Files	Coverage Δ
click_odoo_contrib/initdb.py	`96.95% <100.00%> (ø)`
click_odoo_contrib/manifest.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 478983c...01f6d5e. Read the comment docs.

sbidoul · 2021-04-22T06:56:32Z

Do I need to do anything else to update the changelog?

No that's fine, it will be updated when releasing.

Still I have doubts that auto_install and active mean exactly the same. For instance in the Odoo documentation, they are distinct entries. Is that a documentation bug ? I'd really like a test that reproduces the bug.

joao-p-marques · 2021-04-22T06:59:02Z

Still I have doubts that auto_install and active mean exactly the same. For instance in the Odoo documentation, they are distinct entries. Is that a documentation bug ? I'd really like a test that reproduces the bug.

I think the documentation is buggy, at least from reading the code I mention above.

The test I add should assert that, at least for v13 and v14.

yajo · 2021-08-24T07:01:50Z

I'm patching the docs in odoo/documentation#1109 to clarify this situation. This fix has been in production for months, is all green and has proper docs and tests. IMHO this can be merged.

joao-p-marques · 2021-10-05T12:25:46Z

@sbidoul any news on this? Could it be merged?

sbidoul · 2021-10-05T12:29:51Z

I'm still have doubts. For instance if l10n_fi is active = auto_install = True, why does it not auto install when we install account ?

joao-p-marques · 2021-10-05T12:36:27Z

It has been some time since I've looked into it, but I think what you are saying is exactly what I am doing here.

If active = auto_install = True, the module will be installed if we install account and should be included in the hash.

However, the current behavior, is that it is always included, even if its dependencies are not installed (like account).

sbidoul · 2021-10-05T12:50:23Z

What I mean is that l10n_fi does not auto install in my databases when I do "odoo -i account", so active must be doing something different than auto_install ?

joao-p-marques · 2021-10-05T13:01:55Z

Oh, I see... I think it is because Odoo defaults auto_install to False here and because of that, never actually gets here to read the active property.

I guess that is another bug, as by looking at the code you can see they should have the same behavior...

In that case, we can either leave this as is, or simply ignore the active keyword completely, as Odoo clearly does the same... The current behavior in click-odoo-contrib, however, is not correct IMHO.

yajo · 2021-12-23T07:23:18Z

It seems the docs have settled in odoo/documentation#1427. I'd say the patch is incomplete because it doesn't contemplate situations where active or auto_install is an empty or a non-empty list of modules, where behaviours would be still different.

joao-p-marques · 2021-12-23T09:20:54Z

Thanks for the reference @yajo

I will work on the necessary changes 👍

joao-p-marques · 2021-12-23T10:05:57Z

@yajo @sbidoul taking into account the latest updates to the documentation, and also what I mentioned in #93 (comment) about the actual usage of the active keyword, I ended up:

Ignoring the active keyword completely, as Odoo does the same. For further reference, AFAICS Odoo completely removes the active keyword in v15: https://github.com/odoo/odoo/blob/15.0/odoo/modules/module.py
Applying the needed patch to handle the cases where auto_install is a set, although it was not possible to make a test as Odoo doesn't have a single implementation of it in the current modules.

…orrectly According to the updated documentation, active=True in a module's manifest is a deprecated equivalent to auto_install=True: odoo/odoo@2cc7f0d#diff-367842087e68d6811cf53f2ab3eb21103cd8e793b1648ef3e57f220ee0ccde4eL345 https://www.odoo.com/documentation/15.0/developer/reference/backend/module.html it does not mean that Odoo installs it automatically, as mentioned in acsone#9 OTOH, currently Odoo ignores it. See: https://github.com/odoo/odoo/blob/13.0/odoo/modules/module.py#L335 https://github.com/odoo/odoo/blob/13.0/odoo/modules/module.py#L370 So, we should ignore that key. Besides, this implements the more complex functionality of defining auto_install as a subset of the module's dependencies, as also explained in the documentation. Also, add a new simple test for this case.

yajo · 2022-02-02T09:13:17Z

This has been more than half a year in production. Can't we merge?

yajo · 2022-03-17T12:05:14Z

Hello @sbidoul! 👋🏼

Any chances for a new review and merge?

pedrobaeza · 2022-10-06T13:33:05Z

@sbidoul coming again to this. Can this be merged?

sbidoul · 2022-10-06T15:09:25Z

click_odoo_contrib/manifest.py

    if include_auto_install:
        auto_install_list = []
        for module_name in sorted(odoo.modules.module.get_modules()):
            module_path = odoo.modules.get_module_path(module_name)
            manifest = read_manifest(module_path)
-            if manifest.get("auto_install"):
+            # "auto_install" can be a list or a boolean
+            # see Odoo's documentation for Module Manifests


This part seems unrelated to the issue, and should be best done in another PR.
BTW, where is this documented?

NVM, I found the comment in the Odoo codebase.

Tell me and I put the link here as reference.

sbidoul · 2022-10-06T15:52:33Z

newsfragments/93.bugfix

@@ -0,0 +1,3 @@
+Fix dependencies expansion: handle active=True correctly
+`active=True` in a module's manifest is a deprecated equivalent to `auto_install=True`,
+so we should handle that key the same way we handle the `auto_install` one.


This description is incorrect? If I read the PR correctly, it ignores the active flag so it does not handle it the same way as auto_install.

sbidoul · 2022-10-06T15:55:12Z

click_odoo_contrib/manifest.py

+            if (
+                isinstance(auto_install, list)
+                and (
+                    manifest.get("auto_install") == []


This does not look right. If auto_install is an empty list, the module should be installed unconditionally, so in that case it should be add_deps(module_name).

sbidoul · 2022-10-06T16:00:54Z

@pedrobaeza I read this again and I found a couple of issues, I think.

Also, it seem Odoo 12 still supported the active flag, so we should skip the check for active for Odoo >= 13.

pedrobaeza · 2022-10-06T16:03:03Z

OK, seeing that this is not direct at all, we can leave it on hold, and I'll come back to this later with less workload, and also not being something terrible. I haven't found a manifest with active for years...

Thank you very much for reviewing it. I'll tell you when the changes are done.

sbidoul · 2022-10-06T16:06:19Z

I haven't found a manifest with active for years...

Indeed :) Actually I think the real bug is that we don't support auto_install=[]. I'll create an issue for that.

(cherry picked from commit 59264de)

joao-p-marques mentioned this pull request Apr 15, 2021

Improve install and test tasks Tecnativa/doodba-copier-template#236

Merged

1 task

joao-p-marques force-pushed the fix-active-modules-hashing branch from da36cee to 1abcc06 Compare April 15, 2021 13:35

yajo approved these changes Apr 16, 2021

View reviewed changes

joao-p-marques added a commit to Tecnativa/doodba-copier-template that referenced this pull request Apr 16, 2021

test: Install click-odoo from acsone/click-odoo-contrib#93 to test

dc3f8ed

joao-p-marques added a commit to Tecnativa/doodba-copier-template that referenced this pull request Apr 16, 2021

test: Install click-odoo from acsone/click-odoo-contrib#93 to test

dafc6dc

joao-p-marques added a commit to Tecnativa/doodba-copier-template that referenced this pull request Apr 19, 2021

test: Install click-odoo from acsone/click-odoo-contrib#93 to test

0fc7af8

joao-p-marques added a commit to Tecnativa/doodba-copier-template that referenced this pull request Apr 20, 2021

test: Install click-odoo from acsone/click-odoo-contrib#93 to test

5c0e5f1

joao-p-marques added a commit to Tecnativa/doodba that referenced this pull request Apr 20, 2021

Install click-odoo from acsone/click-odoo-contrib#93 to fix DB hash

37e9eb4

joao-p-marques mentioned this pull request Apr 20, 2021

Fix pylint-odoo and click-odoo installation Tecnativa/doodba#414

Merged

joao-p-marques added a commit to Tecnativa/doodba that referenced this pull request Apr 20, 2021

Install click-odoo from acsone/click-odoo-contrib#93 to fix DB hash

985387e

yajo pushed a commit to Tecnativa/doodba that referenced this pull request Apr 20, 2021

Install click-odoo from acsone/click-odoo-contrib#93 to fix DB hash

59264de

joao-p-marques force-pushed the fix-active-modules-hashing branch from 1abcc06 to 2f0a3c2 Compare April 22, 2021 05:50

joao-p-marques force-pushed the fix-active-modules-hashing branch from 2f0a3c2 to 8482199 Compare December 23, 2021 10:02

joao-p-marques force-pushed the fix-active-modules-hashing branch from 8482199 to 01f6d5e Compare December 23, 2021 10:23

sbidoul reviewed Oct 6, 2022

View reviewed changes

sbidoul mentioned this pull request Oct 6, 2022

click-odoo-update/initdb: support auto_install=[] #121

Open

theangryangel mentioned this pull request Mar 6, 2023

[FIX][16.0] click-odoo-contrib so out of date it doesnt understand 16.0 Tecnativa/doodba#541

Merged

sbidoul added the WIP label Sep 2, 2023

IJOL pushed a commit to BITVAX/doodba that referenced this pull request Apr 16, 2024

Install click-odoo from acsone/click-odoo-contrib#93 to fix DB hash

4e51f39

(cherry picked from commit 59264de)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dependencies expansion: handle active=True correctly #93

Fix dependencies expansion: handle active=True correctly #93

joao-p-marques commented Apr 15, 2021

joao-p-marques commented Apr 15, 2021

sbidoul commented Apr 15, 2021

joao-p-marques commented Apr 15, 2021 •

edited

Loading

sbidoul commented Apr 21, 2021

joao-p-marques commented Apr 22, 2021

codecov-commenter commented Apr 22, 2021 •

edited

Loading

sbidoul commented Apr 22, 2021

joao-p-marques commented Apr 22, 2021

yajo commented Aug 24, 2021

joao-p-marques commented Oct 5, 2021

sbidoul commented Oct 5, 2021

joao-p-marques commented Oct 5, 2021

sbidoul commented Oct 5, 2021

joao-p-marques commented Oct 5, 2021

yajo commented Dec 23, 2021

joao-p-marques commented Dec 23, 2021

joao-p-marques commented Dec 23, 2021

yajo commented Feb 2, 2022

yajo commented Mar 17, 2022

pedrobaeza commented Oct 6, 2022

sbidoul Oct 6, 2022

sbidoul Oct 6, 2022

pedrobaeza Oct 6, 2022

sbidoul Oct 6, 2022

sbidoul Oct 6, 2022

sbidoul commented Oct 6, 2022

pedrobaeza commented Oct 6, 2022

sbidoul commented Oct 6, 2022 •

edited

Loading

Fix dependencies expansion: handle active=True correctly #93

Are you sure you want to change the base?

Fix dependencies expansion: handle active=True correctly #93

Conversation

joao-p-marques commented Apr 15, 2021

To reproduce

joao-p-marques commented Apr 15, 2021

sbidoul commented Apr 15, 2021

joao-p-marques commented Apr 15, 2021 • edited Loading

sbidoul commented Apr 21, 2021

joao-p-marques commented Apr 22, 2021

codecov-commenter commented Apr 22, 2021 • edited Loading

Codecov Report

sbidoul commented Apr 22, 2021

joao-p-marques commented Apr 22, 2021

yajo commented Aug 24, 2021

joao-p-marques commented Oct 5, 2021

sbidoul commented Oct 5, 2021

joao-p-marques commented Oct 5, 2021

sbidoul commented Oct 5, 2021

joao-p-marques commented Oct 5, 2021

yajo commented Dec 23, 2021

joao-p-marques commented Dec 23, 2021

joao-p-marques commented Dec 23, 2021

yajo commented Feb 2, 2022

yajo commented Mar 17, 2022

pedrobaeza commented Oct 6, 2022

sbidoul Oct 6, 2022

Choose a reason for hiding this comment

sbidoul Oct 6, 2022

Choose a reason for hiding this comment

pedrobaeza Oct 6, 2022

Choose a reason for hiding this comment

sbidoul Oct 6, 2022

Choose a reason for hiding this comment

sbidoul Oct 6, 2022

Choose a reason for hiding this comment

sbidoul commented Oct 6, 2022

pedrobaeza commented Oct 6, 2022

sbidoul commented Oct 6, 2022 • edited Loading

joao-p-marques commented Apr 15, 2021 •

edited

Loading

codecov-commenter commented Apr 22, 2021 •

edited

Loading

sbidoul commented Oct 6, 2022 •

edited

Loading