-
Notifications
You must be signed in to change notification settings - Fork 42
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
[Doc] Remove temporary workaround of venv usage for Ubuntu 24.04 #217
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @adarshan-intel)
Suggestion:
[Docs]
Documentation/index.rst
line 62 at r1 (raw file):
sudo apt update && sudo apt install -y docker.io python3 \ 'python3-docker=5.0.3-1ubuntu1.1' python3-jinja2 python3-toml \
Do we need to pin the specific version? The currently available one is good, and pinning will prevent updates to newer versions.
Code quote:
'python3-docker=5.0.3-1ubuntu1.1'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @adarshan-intel and @mkow)
Documentation/index.rst
line 51 at r1 (raw file):
addition, install the Docker client, Jinja2, TOML, and YAML python packages via pip. GSC requires Python 3.6 or later.
For consistency, please add smth a line like For Ubuntu 22.04:
Documentation/index.rst
line 54 at r1 (raw file):
.. code-block:: sh sudo apt-get install docker.io python3 python3-pip
For consistency, replace this line with:
sudo apt update && sudo apt install -y docker.io python3 python3-pip
Documentation/index.rst
line 62 at r1 (raw file):
sudo apt update && sudo apt install -y docker.io python3 \ 'python3-docker=5.0.3-1ubuntu1.1' python3-jinja2 python3-toml \
python3-toml
? This looks like a typo. Must be python3-tomli
Documentation/index.rst
line 62 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Do we need to pin the specific version? The currently available one is good, and pinning will prevent updates to newer versions.
+1, why specific version?
0696246
to
48d4199
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)
-- commits
line 2 at r1:
Done.
Documentation/index.rst
line 51 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
For consistency, please add smth a line like
For Ubuntu 22.04:
Done.
Documentation/index.rst
line 54 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
For consistency, replace this line with:
sudo apt update && sudo apt install -y docker.io python3 python3-pip
Done.
Documentation/index.rst
line 62 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1, why specific version?
Alright, instead of pinning it, I’ll keep it to python3-docker
itself.
Documentation/index.rst
line 62 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
python3-toml
? This looks like a typo. Must bepython3-tomli
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel, @dimakuv, and @mkow)
Documentation/index.rst
line 42 at r2 (raw file):
============= The installation descriptions of prerequisites are for Ubuntu 20.04 and may
The installation descriptions of prerequisites are for Ubuntu 20.04
we don't need it as ubuntu22.04
is mentioned below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @jkr0103 and @mkow)
Documentation/index.rst
line 42 at r2 (raw file):
Previously, jkr0103 (Jitender Kumar) wrote…
The installation descriptions of prerequisites are for Ubuntu 20.04
we don't need it asubuntu22.04
is mentioned below?
True, this whole sentence can be removed I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @jkr0103, and @mkow)
Documentation/index.rst
line 42 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
True, this whole sentence can be removed I think.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel)
Documentation/index.rst
line 53 at r3 (raw file):
.. code-block:: sh sudo apt update && sudo apt install -y docker.io python3 python3-pip
You should keep apt-get
here, apt
doesn't have a stable interface.
Documentation/index.rst
line 60 at r3 (raw file):
.. code-block:: sh sudo apt update && sudo apt install -y docker.io python3 \
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
Documentation/index.rst
line 53 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
You should keep
apt-get
here,apt
doesn't have a stable interface.
Done.
Documentation/index.rst
line 60 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
dae4792
to
c306c4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
This commit reverts 3168b08 "[Docs] Add venv workaround for Ubuntu 24.04" as the proper fix for compatibility issues of Docker SDK for Python on Ubuntu 24.04 was upstreamed. Signed-off-by: Adarsh Anand <[email protected]>
c306c4f
to
151d431
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
Description of the changes
The fix for the issue reported in #202 has been upstreamed. This PR removes the previous workaround involving virtual environments (venv) for Ubuntu 24.04, which was necessary due to compatibility issues with the Docker SDK for Python.
Users running Ubuntu 24.04 are now instructed to follow the updated docs to install packages for
gsc build
andsign
commands.This workaround was proposed to be removed once the
python3-docker
package was fixed via updates, and since Ubuntu 24.04 has received this fix, we are now removing it.Fixes #202
How to test this PR?
Run the
gsc build
andgsc sign
commands.This change is