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.

Sign up for GitHub

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

Jump to bottom

Generate requirements.txt #98

Closed
sbidoul opened this issue Jan 13, 2020 · 6 comments
Closed

Generate requirements.txt #98

sbidoul opened this issue Jan 13, 2020 · 6 comments
Labels
Bot Task A task the bot does or should do enhancement New feature or request

Comments

@sbidoul
Copy link
Member

sbidoul commented Jan 13, 2020 •
edited
Loading

The repo-level requirements.txt could be generated from addons external dependencies.

Benefits:

  • ensure the quality of external dependencies declarations in addons
  • less redundant manual work to maintain this file

Drawbacks

  • in a transition period, this will reveal a lot of repos where external dependencies are not declared properly
@sbidoul sbidoul added enhancement New feature or request Bot Task A task the bot does or should do labels Jan 13, 2020
@sbidoul sbidoul mentioned this issue Jan 13, 2020
Closed
@simahawk
Copy link

@sbidoul nice idea. I wonder if we can do the same with oca_dependencies.txt.
It's a bit more complex but not impossible I think.
The only issue I see is: how can we specify temporary overrides for PR?
Maybe w/ a specific file like oca_dependencies_override.txt?

@sbidoul
Copy link
Member Author

sbidoul commented Jan 13, 2020 •
edited
Loading

We could get rid of oca_dependencies.txt if travis would install dependencies with pip. There is an old PR of mine that was doing just this: OCA/maintainer-quality-tools#343
Temporary overrides can be done by updating requirements.txt with git branch requirements such as -e git+https://github.com/oca/repo@refs/pull/PR/head#egg=odoo12-addon-my_addon&subdirectory=setup/my_addon.

guewen added a commit to guewen/maintainer-quality-tools that referenced this issue Jan 27, 2020
@guewen
Add isort third party detection from requirements.txt
3f4d743 
The current setup used in the OCA for isort is done by using 2
pre-commit hooks:

1. seed-isort-config: find third-party libs and updates the
'.isort.cfg' file's "known_third_party" option with the list
2. isort: runs isort, which will use the list of third party
libs provided by 'seed-isort-config' in the "known_third_party" option.

This is an issue [0] with the workflow adopted by many contributors of
the OCA: as the pull requests may need some time to be merged, we use
temporary branches where we aggregate the various PRs.

If several pull requests add a third party library, we'll have a
conflict, and when the process of merging these pull requests is
automated (e.g. with git-aggregator [1]), it becomes tedious.

The list of libs in the "known_third_party" variable is not even
editable manually, as "seed-isort-config" overwrite the value on every
commit: it doesn't really make sense to actually store this.

As pointed out by @sbidoul [2], isort mentions in their changelog:

> Support for using requirements files to auto determine third-paty
> section if pipreqs & requirementslib are installed.

Which is the goal of this change, details:

* Remove 'seed-isort-config' from pre-commit as the list of libs will be
  provided by requirements.txt
* Replace the isort pre-commit repo by the official repo (the mirror says
  it is been deprecated in favor of the official repo)
* Adds pipreqs and pip-api in additional dependencies to activate
  isort's feature to read requirements.txt [3]
* Adds types: [python] in the pre-commit config: the mirror had it and
  the official repo doesn't. Without it, some files such as .pot are
  modified (different EOF). (it can be removed after the next isort
  release)
* Add an union merge driver on 'requirements.txt' to resolve conflicts
  on this file (on conflicts, both lines are kept, which works in most
  cases on this file, in the rare case it's an issue because we have
  2 requirements for different versions, we'll have to fix manually)

Note: it is now important to have the Python libraries declared in
"requirements.txt" to have them ordered in the proper place. At some
point, this file could be automatically generated: [4].

Alternatively, we could have tried 'reorder-python-imports' in place of 'isort' [5].

This setup has been tested successfully on OCA/stock-logistics-warehouse#828

[0] OCA#625
[1] https://github.com/acsone/git-aggregator
[2] OCA#625 (comment)
[3] https://github.com/timothycrosley/isort/blob/500bafabbd51a6005c11a00c4738a2438990e48a/pyproject.toml#L42
[4] OCA/oca-github-bot#98
[5] https://github.com/asottile/reorder_python_imports

Closes OCA#625
guewen added a commit to guewen/maintainer-quality-tools that referenced this issue Jan 31, 2020
@guewen
Add isort third party detection from requirements.txt
7c98c8b 
The current setup used in the OCA for isort is done by using 2
pre-commit hooks:

1. seed-isort-config: find third-party libs and updates the
'.isort.cfg' file's "known_third_party" option with the list
2. isort: runs isort, which will use the list of third party
libs provided by 'seed-isort-config' in the "known_third_party" option.

This is an issue [0] with the workflow adopted by many contributors of
the OCA: as the pull requests may need some time to be merged, we use
temporary branches where we aggregate the various PRs.

If several pull requests add a third party library, we'll have a
conflict, and when the process of merging these pull requests is
automated (e.g. with git-aggregator [1]), it becomes tedious.

The list of libs in the "known_third_party" variable is not even
editable manually, as "seed-isort-config" overwrite the value on every
commit: it doesn't really make sense to actually store this.

As pointed out by @sbidoul [2], isort mentions in their changelog:

> Support for using requirements files to auto determine third-paty
> section if pipreqs & requirementslib are installed.

Which is the goal of this change, details:

* Remove 'seed-isort-config' from pre-commit as the list of libs will be
  provided by requirements.txt
* Replace the isort pre-commit repo by the official repo (the mirror says
  it is been deprecated in favor of the official repo)
* Adds pipreqs and pip-api in additional dependencies to activate
  isort's feature to read requirements.txt [3]
* Adds types: [python] in the pre-commit config: the mirror had it and
  the official repo doesn't. Without it, some files such as .pot are
  modified (different EOF). (it can be removed after the next isort
  release)
* Add an union merge driver on 'requirements.txt' to resolve conflicts
  on this file (on conflicts, both lines are kept, which works in most
  cases on this file, in the rare case it's an issue because we have
  2 requirements for different versions, we'll have to fix manually)

Note: it is now important to have the Python libraries declared in
"requirements.txt" to have them ordered in the proper place. At some
point, this file could be automatically generated: [4].

Alternatively, we could have tried 'reorder-python-imports' in place of 'isort' [5].

This setup has been tested successfully on OCA/stock-logistics-warehouse#828

[0] OCA#625
[1] https://github.com/acsone/git-aggregator
[2] OCA#625 (comment)
[3] https://github.com/timothycrosley/isort/blob/500bafabbd51a6005c11a00c4738a2438990e48a/pyproject.toml#L42
[4] OCA/oca-github-bot#98
[5] https://github.com/asottile/reorder_python_imports

Closes OCA#625
@sbidoul
Copy link
Member Author

sbidoul commented Mar 24, 2020

FTR, adding MQT_DEP=PIP now enables an alternate installation of addons to test and their dependencies, using the dependencies recorded in manifests (and possibly refined with version constraints in setup.py). This method does not use requirements.txt nor oca_dependencies.txt.

@legalsylvain
Copy link
Collaborator

legalsylvain commented Mar 29, 2020 •
edited
Loading

FTR, adding MQT_DEP=PIP now enables an alternate installation of addons to test and their dependencies, using the dependencies recorded in manifests (and possibly refined with version constraints in setup.py). This method does not use requirements.txt nor oca_dependencies.txt.

if travis doesn't require requirements.txt nor oca_dependencies.txt, this issue will be obsolete ?

Do you know how runbot works ? is it also based on there two files ?

@sbidoul
Copy link
Member Author

sbidoul commented Mar 29, 2020

if travis doesn't requires requirements.txt nor oca_dependencies.txt, this issue will be obsolete ?

Yes

Do you know how runbot works ? is it also based on there two files ?

I think OCA's runbot generates a docker image from travis.yml using https://github.com/vauxoo/travis2docker. So if it works in travis it should work in runbot too. In reality there are probably edge cases but I guess these are fixable.

@sbidoul
Copy link
Member Author

sbidoul commented Mar 29, 2020

So let's close this, we have a better path forward.

@sbidoul sbidoul closed this as completed Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bot Task A task the bot does or should do enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@simahawk @sbidoul @legalsylvain