Skip to content

Conversation

@dougbu
Copy link
Contributor

@dougbu dougbu commented Mar 2, 2025

  • give helixbot user UID 1000 in Ubuntu 24.04 and 24.10 images
    • delete the ubuntu user to free up that UID
  • always provide and activate a venv for helixbot user
  • always install helix-scripts module and dependencies in venv
    • some images installed this module in the system Python environment
    • where possible, do this installation as helixbot user
  • upgrade some Python OS package versions
    • avoid some older versions e.g. python3.8
    • link new version to /usr/bin/python
  • upgrade pip Python package in some images
  • use ${VIRTUAL_ENV}/bin/pip if run for venv before $PATH is changed
    • avoid updating system environment in some cases, defence in depth in others
  • install sudo package in Mariner 2.0 and Azure Linux 3.0 images
  • update /etc/sudoers.d/helixbot instead of /etc/sudoers everywhere
  • nits:
    • remove helixbot2 users; they no longer help anything
    • remove virtualenv module and comments it
    • remove helix-scripts wheels after installing them
    • make some syntax a bit more consistent

@dougbu dougbu requested review from a team as code owners March 2, 2025 02:20
@dougbu dougbu requested a review from richlander March 3, 2025 06:37
@dougbu
Copy link
Contributor Author

dougbu commented Mar 3, 2025

build is green and I've done some local validation. ready for review…

procps-ng \
python3 \
python3-devel \
python3.12 \
Copy link
Member

Choose a reason for hiding this comment

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

Versioning pinning Python is a new practice. It would be good to include why this is needed and what other Dockerfile maintainers should do going forward, in the PR description.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm not sure why this is being done. The version associated with python3 is 3.9 which is the same version that exists in some other Helix images. So why does this need to be higher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Versioning pinning Python is a new practice

not really. it was used already in src/opensuse/15.6/helix/amd64/Dockerfile

the background is fairly simple: OS vendors sometimes have Python dependencies of their own or leave the Python packages alone after a release. this means our software can easily fall behind Python's deprecations and we have to support a broader range of Python versions than is manageable

the versioned packages move forward and are sometimes backported to prior releases, reducing our costs. see below for more about Centos Stream 9 in particular (bumping Python elsewhere in this PR e.g. on Debian 11 was required)


here and in src/centos-stream/9/helix/amd64/Dockerfile, this update avoided some messier workarounds for problems installing helix-scripts dependencies when using Python 3.9. I don't remember the exact details but believe this was the most straightforward fix

that said

  1. 3.9 will be out of support in October IIRC
  2. I'll try the python -m pip install --upgrade pip workaround used in src/debian/11/helix/arm64v8/Dockerfile (where 3.9 is the newest available Python)

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to document in this repo (not in this issue):

  • Min required Python version
  • Encouraged patterns for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the words need to land somewhere other than this PR. where would be appropriate❓


for future reference and given pushback here, I think the rules are something like:

  • try other workarounds b/4 moving to non-default OS packages for Python when problems arise
  • if either ^^ is not possible or the default OS packages for Python install a deprecated version, move to the latest available Python 3 packages that are for released (not Beta or RC) versions
  • today, minimum Python version should be 3.9

patterns when "pinning" is needed include

  • pin version number for all python3* packages in affected Dockerfile unless unavailable for an OS e.g. python312-pip exists for SUSE 15.6 but only python3-pip exists for Ubuntu 20.04 (though python3.9-dev is what we're using now)
  • don't include two Python versions in the OS packages list i.e., change the OS package list and don't add to it
    • if OS itself depends on deprecated / busted-for-us Python, this will be additive where it wasn't b/4 and that's fine
  • link /usr/bin/python to specific /usr/bin/python3.{number} executable matching "pinned" Python version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I resolved original comments in current iteration

Copy link
Member

Choose a reason for hiding this comment

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

I think start with an issue on what the model should be for #1374 (comment). After we lock that down, we can leave it as an issue and link to it or move it to a repo doc.

procps-ng \
python3 \
python3-devel \
python3.12 \
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm not sure why this is being done. The version associated with python3 is 3.9 which is the same version that exists in some other Helix images. So why does this need to be higher?

- give `helixbot` user UID 1000 in Ubuntu 24.04 and 24.10 images
  - delete the `ubuntu` user to free up that UID
- always provide and activate a `venv` for `helixbot` user
- always install `helix-scripts` module and dependencies in `venv`
  - some images installed this module in the system Python environment
  - where possible, do this installation as `helixbot` user
- upgrade some Python OS package versions
  - avoid some older versions e.g. `python3.8`
  - link new version to /usr/bin/python
- upgrade `pip` Python package in some images
  - work around https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1003252
  - bug caused problems installing `psutil` package
- use `${VIRTUAL_ENV}/bin/pip` if run for `venv` before `$PATH` is changed
  - avoid updating system environment in some cases, defence in depth in others
- install `sudo` package in Mariner 2.0 and Azure Linux 3.0 images
- update /etc/sudoers.d/helixbot instead of /etc/sudoers everywhere
- nits:
  - remove `helixbot2` users; they no longer help anything
  - remove `virtualenv` module and comments it
  - remove `helix-scripts` wheels after installing them
  - make some syntax a bit more consistent
@dougbu dougbu enabled auto-merge March 5, 2025 20:58
@dougbu dougbu merged commit 8bf623f into dotnet:main Mar 5, 2025
46 checks passed
@dougbu dougbu deleted the dougbu/helix.cleanup branch March 5, 2025 21:56
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