Skip to content

Conversation

@remybar
Copy link

@remybar remybar commented Nov 24, 2021

In conf.py, we try to find a Odoo sources folder among odoo and ../odoo directories without really checking that they really are
Odoo sources folders, leading to doc generation error if it's not the case.

So, to improve that, this commit checks that the selected Odoo sources folder contains odoo-bin.

@remybar remybar requested a review from a team as a code owner November 24, 2021 09:52
@robodoo
Copy link
Collaborator

robodoo commented Nov 24, 2021

Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with the change but, since we keep patching the patch that allows building the doc on any setup, I also think it's time to introduce a config option to specify the odoo sources paths. It would look like this in the CLI: make ODOO_SOURCE_PATH=../odoo. If not passed, the default should be odoo to avoid breaking current setups. I'll create a task to do it later.

@remybar
Copy link
Author

remybar commented Nov 24, 2021

I'm okay with the change but, since we keep patching the patch that allows building the doc on any setup, I also think it's time to introduce a config option to specify the odoo sources paths. It would look like this in the CLI: make ODOO_SOURCE_PATH=../odoo. If not passed, the default should be odoo to avoid breaking current setups. I'll create a task to do it later.

I tried to do that but, as far as I know, in the Makefile we can just override some variables set by conf.py (using the -D option of sphinx-build). I didn't find any possibility to provide this ODOO_SOURCE_PATH argument to sphinx-build and being able to access to it inside conf.py.

Thanks for the review, I will update the PR ;-)

@AntoineVDV
Copy link
Collaborator

I tried to do that but, as far as I know, in the Makefile we can just override some variables set by conf.py (using the -D option of sphinx-build). I didn't find any possibility to provide this ODOO_SOURCE_PATH argument to sphinx-build and being able to access to it inside conf.py.

You need to declare it in def setup(app) like this: app.add_config_value('odoo_source_path', None, 'env'). Then, you can pass -D odoo_source_path=$(ODOO_SOURCE_PATH) to sphinx-build in the Makefile and read the config value with app.config.odoo_source_path in conf.py.

@remybar remybar force-pushed the 13.0-check_odoo_folder_contains_odoo_sources-bar branch from d01f52d to 2b36476 Compare November 25, 2021 08:39
@remybar
Copy link
Author

remybar commented Nov 25, 2021

I tried to do that but, as far as I know, in the Makefile we can just override some variables set by conf.py (using the -D option of sphinx-build). I didn't find any possibility to provide this ODOO_SOURCE_PATH argument to sphinx-build and being able to access to it inside conf.py.

You need to declare it in def setup(app) like this: app.add_config_value('odoo_source_path', None, 'env'). Then, you can pass -D odoo_source_path=$(ODOO_SOURCE_PATH) to sphinx-build in the Makefile and read the config value with app.config.odoo_source_path in conf.py.

So, after some tests, unfortunately, it's not so easy to dynamically configure extensions in conf.py using a ODOO_SOURCE_PATH provided through -D option to sphinx-build, even using the config-inited signal.

But, in fact, we have 2 use cases:

  1. users without Odoo sources: if not sources are found in odoo and ../odoo a warning will be displayed and the doc will be correctly generated => OK.
  2. users with Odoo sources: if sources are found in odoo or ../odoo, they will be taken into account during doc generation. If not found, same than 1). If the user uses a "non standard" location for his Odoo sources, he can create a odoo symlink to refer to his source location.
    So, at the end, no specific need to handle a custom odoo source path through a Makefile parameter.

@remybar remybar requested a review from AntoineVDV November 25, 2021 08:48
In `conf.py`, we try to find a Odoo sources folder among `odoo` and
`../odoo` directories without really checking that they really are
Odoo sources folders, leading to doc generation error if it's not the case.

So, to improve that, this commit checks that the selected Odoo sources folder
contains `odoo-bin`.

While we're at it, the logging is also improved to always display the
matching odoo sources' folder and version.

Co-authored-by: Antoine Vandevenne <anv@odoo.com>
@AntoineVDV AntoineVDV force-pushed the 13.0-check_odoo_folder_contains_odoo_sources-bar branch from 2b36476 to 0e96c53 Compare November 26, 2021 11:21
@AntoineVDV
Copy link
Collaborator

LGTM!

FYI: I took the liberty to improve the logging messages a bit, now that it's clear that we support alternative odoo sources paths.

@robodoo r+

robodoo pushed a commit that referenced this pull request Nov 26, 2021
In `conf.py`, we try to find a Odoo sources folder among `odoo` and
`../odoo` directories without really checking that they really are
Odoo sources folders, leading to doc generation error if it's not the case.

So, to improve that, this commit checks that the selected Odoo sources folder
contains `odoo-bin`.

While we're at it, the logging is also improved to always display the
matching odoo sources' folder and version.

closes #1333

Signed-off-by: Antoine Vandevenne (anv) <anv@odoo.com>
Co-authored-by: Antoine Vandevenne <anv@odoo.com>
@robodoo robodoo closed this Nov 26, 2021
@robodoo robodoo temporarily deployed to merge November 26, 2021 11:31 Inactive
@fw-bot fw-bot deleted the 13.0-check_odoo_folder_contains_odoo_sources-bar branch December 10, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants