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

Stop manually installing python #7613

Merged
merged 1 commit into from
Aug 17, 2023
Merged

Conversation

jeffwidman
Copy link
Member

Depends on:

In the python dockerfile, we pre-install python 3.7-3.11, then tar it
to save space.

Now that we're not supporting python <= 3.6, we no longer have any
reason to run pyenv install, so we can dop that and simplify our code.

@jeffwidman jeffwidman added the Ecosystems Used by the maintainer team for internal-facing project tracking label Jul 24, 2023
@jeffwidman jeffwidman self-assigned this Jul 24, 2023
@jeffwidman jeffwidman force-pushed the stop-manually-installing-python branch 3 times, most recently from b039d80 to c57085a Compare August 2, 2023 23:30
@jeffwidman jeffwidman added L: elixir:hex Elixir packages via hex and removed L: python labels Aug 3, 2023
@jeffwidman jeffwidman force-pushed the stop-manually-installing-python branch from c57085a to f273b22 Compare August 4, 2023 18:57
@github-actions github-actions bot added L: python and removed L: elixir:hex Elixir packages via hex labels Aug 4, 2023
@jeffwidman jeffwidman force-pushed the stop-manually-installing-python branch from f273b22 to ea0d3fc Compare August 10, 2023 00:12
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Seems straightforward but I did have a question!

Dependabot.logger.info("Installing Python #{python_version} took #{time_taken}s.")
SharedHelpers.run_shell_command(
"tar xzf /usr/local/.pyenv/#{python_major_minor}.tar.gz -C /usr/local/.pyenv/"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we find a python version higher than the latest that we support, for example, right after a new Python release.

Copy link
Member Author

@jeffwidman jeffwidman Aug 17, 2023

Choose a reason for hiding this comment

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

Great question!

The behavior should remain the same as today:

  1. Newer patch releases are ignored and we use the matching major.minor pre-installed, until someone bumps the patch release we use (more details in Stop explicitly specifying python patch versions #7615).
  2. Newer minor versions (eg 3.12 that is getting released in a few months) will throw an error about an unsupported Python version.

This is a reasonable case where we'd want to continually to manually install, but OTOH the time/network cost of doing that dynamically on every job that needs it is quite expensive.

So far, it's worked out okay that whenever a new minor release happens, someone from the community pretty quickly puts up an upgrade PR.

Ideally we actually start scheduling this into our ecosystem calendar so that we're ready with a PR the day it gets released. But we're lacking enough maintainer time to stay on top of our deprecations, let alone new versions, so might be a bit til we're that organized.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, all the logic for choosing the python version happens before we even get to this code... it's all happening over in:

# Ideally, the requirement is satisfied by a Python version we support
requirement =
Python::Requirement.requirements_array(requirement_string).first
version =
PythonVersions::SUPPORTED_VERSIONS_TO_ITERATE.
find { |v| requirement.satisfied_by?(Python::Version.new(v)) }
return version if version
# If not, and we're dealing with a simple version string
# and changing the patch version would fix things, we do that
# as the patch version is unlikely to affect resolution
if requirement_string.start_with?(/\d/)
requirement =
Python::Requirement.new(requirement_string.gsub(/\.\d+$/, ".*"))
version =
PythonVersions::SUPPORTED_VERSIONS_TO_ITERATE.
find { |v| requirement.satisfied_by?(Python::Version.new(v)) }
return version if version
end
# Otherwise we have to raise, giving details of the Python versions
# that Dependabot supports
msg = "Dependabot detected the following Python requirement " \
"for your project: '#{requirement_string}'.\n\nCurrently, the " \
"following Python versions are supported in Dependabot: " \
"#{PythonVersions::SUPPORTED_VERSIONS.join(', ')}."
raise DependencyFileNotResolvable, msg

@jeffwidman jeffwidman marked this pull request as ready for review August 17, 2023 06:03
@jeffwidman jeffwidman requested a review from a team as a code owner August 17, 2023 06:03
In the python dockerfile, we pre-install python 3.7-3.11, then `tar` it
to save space.

Now that we're not supporting python <= 3.6, we no longer have any
reason to run `pyenv install`, so we can dop that and simplify our code.
@jeffwidman jeffwidman force-pushed the stop-manually-installing-python branch from ea0d3fc to bfb40cc Compare August 17, 2023 06:14
@jeffwidman jeffwidman enabled auto-merge (squash) August 17, 2023 06:17
@jeffwidman jeffwidman merged commit ab76675 into main Aug 17, 2023
107 checks passed
@jeffwidman jeffwidman deleted the stop-manually-installing-python branch August 17, 2023 06:37
brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
In the python dockerfile, we pre-install python 3.7-3.11, then `tar` it
to save space.

Now that we're not supporting python <= 3.6, we no longer have any
reason to run `pyenv install`, so we can dop that and simplify our code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ecosystems Used by the maintainer team for internal-facing project tracking L: python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants