Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADD] setup: pep518 support #44001

Open
wants to merge 1 commit into
base: 17.0
Choose a base branch
from

Conversation

sbidoul
Copy link
Contributor

@sbidoul sbidoul commented Jan 27, 2020

This is a non-intrusive solution for #16700, that leverages the PEP 517 build backend interface, so pip install . and pip wheel . work out of the box for Odoo.

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Jan 27, 2020
@sbidoul
Copy link
Contributor Author

sbidoul commented Jan 27, 2020

@d-fence

@d-fence d-fence self-requested a review January 27, 2020 15:06
@sbidoul
Copy link
Contributor Author

sbidoul commented May 5, 2020

@d-fence have had a chance to think about this one?

@d-fence
Copy link
Contributor

d-fence commented May 5, 2020

I think that the idea is really attractive but it seems that setuptools 41 is needed.
We try to stick to the Debian stable release (stretch for Odoo 12.0) which provides only 33.
The same idea applies to the included packaging script that uses a Dockerfile corresponding to the current Debian version at the time of the release. I'm afraid that this PR could break the actual packaging system.
We could merge that into master when bullseye become stable.

@sbidoul
Copy link
Contributor Author

sbidoul commented May 5, 2020

@d-fence thanks for looking into this.

Actually the setuptools 41 requirement is the one that has the build_meta PEP 517 API, but it is not needed to run Odoo.

When running pip install . or pip wheel . with a pip version that supports PEP 517, pip will download the build requirements from PyPI in an isolated environment to run the build.

So this should be safe to merge, independently of the distro version for pip and setuptools.

@sbidoul
Copy link
Contributor Author

sbidoul commented Sep 29, 2020

@d-fence would you consider this for v14?

@d-fence
Copy link
Contributor

d-fence commented Sep 29, 2020

Debian buster only provides setup-tools 40.
41 is needed as I read.

@sbidoul
Copy link
Contributor Author

sbidoul commented Sep 29, 2020

But as I mentioned above, this is not needed at runtime nor for debian packaging. And even on buster pip install will download the required setuptools version from PyPI.

@sbidoul
Copy link
Contributor Author

sbidoul commented Sep 30, 2021

@d-fence do you thing this can be considered for v15 ?
What are your remaining concerns ?
If you agree to go ahead I'll do a new pass on this and polish the PR.

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Works like a charm.

Tested on master, so @sbidoul maybe would be accepted more easily on that branch ?

@sbidoul
Copy link
Contributor Author

sbidoul commented Oct 27, 2021

@rousseldenis I'll be more than happy to rebase and improve this if/when someone at Odoo acknowledges it will be considered for inclusion.

@d-fence
Copy link
Contributor

d-fence commented Oct 28, 2021

I have to test it further and yes it would be better to target master.

@sbidoul
Copy link
Contributor Author

sbidoul commented Oct 28, 2021

@d-fence thanks for reaching back ! I'll rebase on master.

I think its no risk for stable though. Let me know if there are specific tests you have in mind, I'm happy to do them too.

@d-fence
Copy link
Contributor

d-fence commented Oct 28, 2021

I want to be sure that it won't clash with our build system and, even better, that it could integrate with our build system.
As you saw, It's not a priority but I'll try to test that soon.

@sbidoul
Copy link
Contributor Author

sbidoul commented Oct 28, 2021

You may want to test building the sdist (and why not a wheel), with PyPA build.

@simahawk
Copy link
Contributor

@d-fence any update?

@sbidoul
Copy link
Contributor Author

sbidoul commented Feb 24, 2022

I cleaned-up and modernized the code, and made it more robust.

@sbidoul sbidoul changed the base branch from 12.0 to master August 28, 2022 16:32
@sbidoul sbidoul changed the base branch from master to 12.0 August 28, 2022 16:33
@sbidoul sbidoul changed the base branch from 12.0 to master August 28, 2022 16:35
@sbidoul
Copy link
Contributor Author

sbidoul commented Aug 28, 2022

@d-fence any chance this can be considered for v16 ? I retargeted this branch to master.

@rousseldenis
Copy link
Contributor

@d-fence gentle reminder. Thanks

@yajo
Copy link
Contributor

yajo commented Apr 20, 2023

I'm getting plenty of warnings like this when building Odoo with this PR enabled:

python3.8-odoo>  SetuptoolsDeprecationWarning:     Installing 'odoo.tools.data.files' as data is deprecated, please list it in `packages`.
python3.8-odoo>       !!
python3.8-odoo>       ############################
python3.8-odoo>       # Package would be ignored #
python3.8-odoo>       ############################
python3.8-odoo>       Python recognizes 'odoo.tools.data.files' as an importable package,
python3.8-odoo>       but it is not listed in the `packages` configuration of setuptools.
python3.8-odoo>       'odoo.tools.data.files' has been automatically added to the distribution only
python3.8-odoo>       because it may contain data files, but this behavior is likely to change
python3.8-odoo>       in future versions of setuptools (and therefore is considered deprecated).
python3.8-odoo>       Please make sure that 'odoo.tools.data.files' is included as a package by using
python3.8-odoo>       the `packages` configuration field or the proper discovery methods
python3.8-odoo>       (for example by using `find_namespace_packages(...)`/`find_namespace:`
python3.8-odoo>       instead of `find_packages(...)`/`find:`).
python3.8-odoo>       You can read more about "package discovery" and "data files" on setuptools
python3.8-odoo>       documentation page.
python3.8-odoo>   !!
python3.8-odoo>     check.warn(importable)
python3.8-odoo>   Building wheel for odoo (pyproject.toml) ... done

Should you add some method to find packages that is more future-proof? 🤔

@sbidoul
Copy link
Contributor Author

sbidoul commented Apr 20, 2023

@yajo these are setuptools deprecation warnings that are not directly related to this PR.
They appear when you do a python setup.py bdist_wheel too.
So that would need a separate PR to update setup.py.

Copy link
Contributor

@KangOl KangOl left a comment

Choose a reason for hiding this comment

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

We should not unlink existing symlink

setup/pep517_odoo.py Outdated Show resolved Hide resolved
@sbidoul sbidoul changed the base branch from master to 17.0 November 9, 2023 08:47
@C3POdoo C3POdoo requested review from a team November 9, 2023 08:48
Allow building a wheel or an sdist through the pep517
interface, taking care of the required pre-processing
to copy addons in odoo/addons.

Force the setuptools compat mode for editable installs.
This mode is the simplest, since it just installs a .pth
files which extends PYTHONPATH with the Odoo root directory.
@sbidoul
Copy link
Contributor Author

sbidoul commented Nov 9, 2023

@KangOl thanks for your review! I updated and rebased on 17 as requested.
pylint complains on the import * but I think it's the right way to do in this case, as not all setuptools versions implement the same build hooks.

@yajo
Copy link
Contributor

yajo commented Jan 29, 2024

Here's the patch for Odoo 16, for those that still need it there.

@sbidoul
Copy link
Contributor Author

sbidoul commented Jan 29, 2024

@KangOl @d-fence gentle nudge here.

@sbidoul
Copy link
Contributor Author

sbidoul commented May 26, 2024

@KangOl @d-fence this PR is becoming more pressing. Indeed there are more and more situations where pip defaults to doing a PEP 660 editable install which triggers a setuptools installation mode that is not compatible with Odoo. It is therefore important to force the compat editable mode, as this PR does.

Pretty please 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants