Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

SEP 20: Add python version support SEP #26

Merged
merged 8 commits into from
May 22, 2020
Merged

Conversation

Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Apr 2, 2020

SEP to update python version support policy for Salt.

@Ch3LL Ch3LL requested a review from a team as a code owner April 2, 2020 19:14
@ghost ghost requested review from waynew and removed request for a team April 2, 2020 19:14
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
@myii
Copy link
Contributor

myii commented Apr 2, 2020

Thanks, a tough issue covered well.

0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
@sblaisot
Copy link

sblaisot commented Apr 3, 2020

As a salt user, I would expect a PEP named "Python Version Support" to not only cover python version deprecation and how salt stop supporting those versions but also what's saltstack goal in supporting newer python versions and how the core team is organized to reach this goal.

This is especially important when dealing with new OS/distribution versions that come with new python version packaged like we see at the moment with python 3.8 and the capacity of salt to support this new OS/distribution versions.

@OrangeDog
Copy link

#26 (comment)
Especially as there are still states that don't yet work on py3.

0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
0000-py-version-support.md Outdated Show resolved Hide resolved
@Ch3LL
Copy link
Contributor Author

Ch3LL commented Apr 3, 2020

@sblaisot good point. I'll add a note that we should check for both python version's being removed or added at the beginning of a development cycle, so we can ensure we add it. To clarify around python 3.8 that will be added in Sodium.

@waynew waynew changed the title Add python version support SEP SEP 20: Add python version support SEP Apr 15, 2020
@waynew
Copy link
Contributor

waynew commented Apr 15, 2020

This is SEP 20 - feel free to update your files :)

@Ch3LL
Copy link
Contributor Author

Ch3LL commented Apr 20, 2020

Can i get re-review here or any further concerns? @twangboy @max-arnold @myii @aplanas @OrangeDog @sblaisot

Copy link

@aplanas aplanas left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@OrangeDog OrangeDog left a comment

Choose a reason for hiding this comment

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

Capitalisation is also inconsistent.

0020-py-version-support.md Outdated Show resolved Hide resolved
0020-py-version-support.md Outdated Show resolved Hide resolved
0020-py-version-support.md Outdated Show resolved Hide resolved
0020-py-version-support.md Outdated Show resolved Hide resolved
0020-py-version-support.md Outdated Show resolved Hide resolved
@Ch3LL
Copy link
Contributor Author

Ch3LL commented Apr 23, 2020

@OrangeDog @myii made updates. ready for re-review. thanks

0020-py-version-support.md Outdated Show resolved Hide resolved
0020-py-version-support.md Outdated Show resolved Hide resolved
@Ch3LL
Copy link
Contributor Author

Ch3LL commented Apr 30, 2020

okay ready for re-review @myii thanks :)

Copy link
Contributor

@myii myii left a comment

Choose a reason for hiding this comment

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

@Ch3LL Thanks for incorporating all of the changes. Just one small thing: "Red Hat" rather than "Red hat", as you can see here:

@Ch3LL
Copy link
Contributor Author

Ch3LL commented Apr 30, 2020

thanks for catching that let me make the H uppercase.

@sagetherage sagetherage assigned waynew and unassigned Ch3LL May 5, 2020
@waynew waynew added the Final Comment Period Speak now or forever hold your peace. label May 11, 2020
@waynew
Copy link
Contributor

waynew commented May 11, 2020

This SEP has entered the final comment period, let us know if there's anything else that needs to be addressed!

@terminalmage
Copy link
Contributor

terminalmage commented May 11, 2020

I like the level of detail WRT different platforms, but there should probably be a matrix listed somewhere on the docs which details which platforms have a bundled Python, which Python release will be bundled, etc.

Additionally, how does adding pip packages work for bundled Python? If these aren't "onedir" POP builds (I didn't see this specified), that makes installing pip packages problematic, as the default method Salt uses to decide which pip to use is to run $python -m pip ....., where $python is sys.executable. I'm not sure what sys.executable returns for a fully-contained build, but it probably won't work with the pip execution module.

I'm not sure any of these concerns merits changes to the contents of this SEP, but at minimum there should be open issues labeled Sodium which ensure that we address them before the release.

@waynew
Copy link
Contributor

waynew commented May 12, 2020

We can still pip install with the one-dir pop-build distributions, so no issue there.

IIUC the plan is to provide pip installable Salt (for which this SEP will be particularly relevant), and then pop-build packages via repo.saltstack.com, which will bundle Python.

@terminalmage
Copy link
Contributor

So, the POP builds will be onedir and not single-binary? That question really wasn't answered. I'm aware of how onedir builds work, the question is an effort to confirm that basic pip functionality will be present in these builds. With so many states and execution modules relying on third-party Python bindings, pip support is pretty important to Salt users.

@OrangeDog
Copy link

OrangeDog commented May 12, 2020

On the subject of bundling...

Many users and formulae use pkg.installed to get salt dependencies from their system packages. This will break all of that then?

@waynew
Copy link
Contributor

waynew commented May 12, 2020

So, the POP builds will be onedir and not single-binary? That question really wasn't answered.

Both.

IIRC, the single-binary can be installed in a onedir fashion. I could be wrong and pop-build has to produce two different packages, but my understanding is that regardless of the mechanics, it's pretty trivial to get both onedir and single-binary.

Many users and formulae use pkg.installed to get salt dependencies from their system packages. This will break all of that then?

Are you referring to module dependencies, or salt core dependencies? Salt core dependencies will be included. But for the module dependencies... my gut says that would break unless we add the system library to the PYTHONPATH.

I'll double check on that.

@OrangeDog
Copy link

OrangeDog commented May 12, 2020

that would break unless we add the system library to the PYTHONPATH

Yes, module dependencies. Also, I don't think that would work unless the system and bundled versions are the same? At which point what was the point of bundling it?

I raised similar issues above but that got shut down because it's for a future SEP. It's not even been proposed yet, but it seems like a forgone conclusion you're going to start bundling python, whatever anyone says.

And if it's supposed to start happening in Sodium, it's rather late to be leaving it...

@waynew
Copy link
Contributor

waynew commented May 12, 2020

Some clarification:

  • onedir and single binary are different artifacts
  • pip installs work already for onedir. There is work happening for pip installs with the single-binary approach.
  • no, system packages module dependencies are not supported.

Also, I don't think that would work unless the system and bundled versions are the same?

only for compiled packages. If it's a pure python package, and I think even if there's an ABI compatibility, but for sure pure python packages would work just fine.

0020-py-version-support.md Outdated Show resolved Hide resolved
The project has been renamed.
@garethgreenaway garethgreenaway self-requested a review May 21, 2020 20:27
@waynew
Copy link
Contributor

waynew commented May 22, 2020

This SEP has received enough votes to be accepted 🎉

@waynew waynew added Accepted and removed Final Comment Period Speak now or forever hold your peace. labels May 22, 2020
@waynew waynew merged commit d3d8902 into saltstack:master May 22, 2020
@sagetherage sagetherage added this to Accepted in SEPherding Feb 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
SEPherding
Accepted