Skip to content

Conversation

@d-fence
Copy link
Contributor

@d-fence d-fence commented Dec 8, 2021

This commit changes the way that Odoo python dependencies should be
installed.

Before this commit, the described way to install Odoo dependencies was
by using the pip utility.

Now, we explicitly recommend to use a Debian based system and install
Odoo dependencies from their packaging tool.

The main reasons are:

  • Odoo productions instances are, most of the time, run on Debian based
    systems with those package already installed
  • That way, a developer cannot mistakenly use a feature from a
    dependency that does not exists on those production envirorments
  • Avoid mixes of python packages on the developer/user 's system

@robodoo
Copy link
Collaborator

robodoo commented Dec 8, 2021

.. tip:: It can be preferable to not mix python modules packages between different instances of Odoo
or with your system. You can use virtualenv_ to create isolated Python environments.
In that case, the `requirements.txt` file can be used to install the dependencies.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we should specify here that -dev packages are required. It is sad to loose this list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That means bringing back the whole pip explanation in the tip ... what do you think @AntoineVDV

@d-fence
Copy link
Contributor Author

d-fence commented Dec 8, 2021

Linked to #1329

Copy link
Member

@amigrave amigrave left a comment

Choose a reason for hiding this comment

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

2 cents

@C3POdoo C3POdoo requested review from a team December 9, 2021 11:00
@d-fence d-fence force-pushed the master-install-from-apt-moc branch from ff0e329 to de702c1 Compare December 10, 2021 09:09
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 find it weird to keep only some of the explanations for the pip setup while removing the others (list of -dev packages, for example). Either we recommend installing the dependencies through apt and we properly explain how to do it through pip for those who prefer, or we only mention the Debian setup. If we go for the first option, I suggest that we properly split the instructions for the two setups in two different headings, and find a nicer-looking solution later on (tabs, maybe).

I'm a bit confused about why we'd want to discourage the pip setup. Maybe @amigrave could enlighten me? And, if we do that, why do we keep (and maintain) a requirements.txt file? Also, are Debian dependencies listed in the debian/control file compatible with any supported version of Odoo?

.. tip:: It can be preferable to not mix python modules packages between different instances of Odoo
or with your system. You can use virtualenv_ to create isolated Python environments.
In that case, the `requirements.txt` file can be used to install the dependencies.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
In that case, the `requirements.txt` file can be used to install the dependencies.
In that case, the :file:`requirements.txt` file can be used to install the dependencies.

@d-fence d-fence force-pushed the master-install-from-apt-moc branch from de702c1 to f8a83c4 Compare December 14, 2021 08:45
@d-fence
Copy link
Contributor Author

d-fence commented Dec 14, 2021

@AntoineVDV I splitted the installation of Python dependencies into debian/Ubuntu and The pip way.
I hope that it will not bring more confusion in the chaos...
And I'm not sure that @amigrave will agree

@amigrave
Copy link
Member

amigrave commented Dec 14, 2021

@AntoineVDV I splitted the installation of Python dependencies into debian/Ubuntu and The pip way. I hope that it will not bring more confusion in the chaos... And I'm not sure that @amigrave will agree

No, I'm ok with that. I'll link from the Odoo.sh FAQ to the "preferred way" of installation.
Maybe mention that the Odoo platforms (Odoo Online and Odoo.sh) uses the debian/ubuntu installation method ? Not mandatory though

@d-fence d-fence force-pushed the master-install-from-apt-moc branch from f8a83c4 to 94e69d0 Compare December 14, 2021 10:12
@AntoineVDV AntoineVDV force-pushed the master-install-from-apt-moc branch 2 times, most recently from 47e2f77 to ba81ba0 Compare December 14, 2021 12:31
@AntoineVDV
Copy link
Collaborator

@AntoineVDV I splitted the installation of Python dependencies into debian/Ubuntu and The pip way. I hope that it will not bring more confusion in the chaos... And I'm not sure that @amigrave will agree

No, I'm ok with that. I'll link from the Odoo.sh FAQ to the "preferred way" of installation. Maybe mention that the Odoo platforms (Odoo Online and Odoo.sh) uses the debian/ubuntu installation method ? Not mandatory though

I don't think that it adds any value from the POV of the doc.

@AntoineVDV AntoineVDV force-pushed the master-install-from-apt-moc branch from ba81ba0 to 867d1ff Compare December 14, 2021 12:55
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 took the liberty to make some fine-tuning changes:

  • RST cleanup.
  • Some wording fixes.
  • Removed the sentence "In that case, the requirements.txt file can be used to install the dependencies." from the virtualenv tip as I believe it's a leftover from when you didn't want to suggest pip anymore.
  • The wkhtmltopdf warning block didn't make sense anymore, I replaced it by a bullet point.

I believe that it's clear enough to be merged but maybe @jcs-odoo wants to do a last pass? Otherwise,
@robodoo delegate=jcs-odoo

@AntoineVDV AntoineVDV force-pushed the master-install-from-apt-moc branch from 867d1ff to ffd1584 Compare December 14, 2021 13:00
@AntoineVDV
Copy link
Collaborator

@d-fence I just noticed two changes that I'm not too sure about:

  • You replaced pip3 with pip in command blocks. Does pip correctly refer to pip3 on the version of Debian/Ubuntu that we support? Because it was not the case before and users had to figure that pip3 was the correct command to call.
  • You removed pip3 install setuptools wheel which I had to add at some point because these packages were not pre-installed with pip on the brand new Linux Mint machines of our newbies. Is this no longer a problem?

@d-fence
Copy link
Contributor Author

d-fence commented Dec 14, 2021

@d-fence I just noticed two changes that I'm not too sure about:

  • You replaced pip3 with pip in command blocks. Does pip correctly refer to pip3 on the version of Debian/Ubuntu that we support? Because it was not the case before and users had to figure that pip3 was the correct command to call.

AFAIK, Buster and Focal only provides python3-pip where pip and pip3 are the same.

  • You removed pip3 install setuptools wheel which I had to add at some point because these packages were not pre-installed with pip on the brand new Linux Mint machines of our newbies. Is this no longer a problem?

In the required libraries, we now ask to install python3-pip which depends and recommends all the necessary packages (setuptools, build-essential ...). And we provide a Mint based on Focal AFAIK.

@jcs-odoo
Copy link
Contributor

I took the liberty to make some fine-tuning changes:

  • RST cleanup.
  • Some wording fixes.
  • Removed the sentence "In that case, the requirements.txt file can be used to install the dependencies." from the virtualenv tip as I believe it's a leftover from when you didn't want to suggest pip anymore.
  • The wkhtmltopdf warning block didn't make sense anymore, I replaced it by a bullet point.

I believe that it's clear enough to be merged but maybe @jcs-odoo wants to do a last pass? Otherwise, @robodoo delegate=jcs-odoo

I'm adding a few small fixes then it's good to go.

This commit changes the way that Odoo python dependencies should be
installed.

Before this commit, the described way to install Odoo dependencies was
by using the `pip` utility.

Now, we explicitly recommend to use a Debian based system and install
Odoo dependencies from their packaging tool.

The main reasons are:
* Odoo productions instances are, most of the time, run on Debian based
  systems with those package already installed
* That way, a developer cannot mistakenly use a feature from a
  dependency that does not exists on those production environments
* Avoid mixes of python packages on the developer/user 's system
@jcs-odoo jcs-odoo force-pushed the master-install-from-apt-moc branch from 33d6c69 to 6ab0d55 Compare December 17, 2021 12:22
@jcs-odoo
Copy link
Contributor

@robodoo r+

robodoo pushed a commit that referenced this pull request Dec 17, 2021
This commit changes the way that Odoo python dependencies should be
installed.

Before this commit, the described way to install Odoo dependencies was
by using the `pip` utility.

Now, we explicitly recommend to use a Debian based system and install
Odoo dependencies from their packaging tool.

The main reasons are:
* Odoo productions instances are, most of the time, run on Debian based
  systems with those package already installed
* That way, a developer cannot mistakenly use a feature from a
  dependency that does not exists on those production environments
* Avoid mixes of python packages on the developer/user 's system

closes #1364

Signed-off-by: Castillo Jonathan (jcs) <jcs@odoo.com>
@robodoo robodoo closed this Dec 17, 2021
@robodoo robodoo temporarily deployed to merge December 17, 2021 12:34 Inactive
@robodoo robodoo added the 15.2 label Dec 17, 2021
@fw-bot fw-bot deleted the master-install-from-apt-moc branch December 31, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants