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

Remove six from production code #12

Merged
merged 1 commit into from
Mar 26, 2024
Merged

Remove six from production code #12

merged 1 commit into from
Mar 26, 2024

Conversation

JPEWdev
Copy link
Collaborator

@JPEWdev JPEWdev commented Mar 22, 2024

Removes the six module from released production code. The old code in the test cases used for testing compatability still uses six, so it is kept as a dev dependency.

@JPEWdev JPEWdev requested review from moto-timo and twoerner March 22, 2024 21:00
@twoerner
Copy link
Collaborator

There are a couple references to python2 in packaging/bmaptool.spec and some of the files in debian. If we're removing python2 support, should those be updated too?

Copy link
Collaborator

@twoerner twoerner left a comment

Choose a reason for hiding this comment

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

Do we need to remove references to python2 in packaging directories?

@JPEWdev
Copy link
Collaborator Author

JPEWdev commented Mar 25, 2024

Probably, but I'm actually not sure what it's going to take to re-write the debian packaging stuff to work "correctly" with the new hatch build?

Removes the `six` module from released production code. The old code in
the test cases used for testing compatability still uses `six`, so it is
kept as a dev dependency.

Signed-off-by: Joshua Watt <[email protected]>
@moto-timo
Copy link
Collaborator

moto-timo commented Mar 25, 2024

Probably, but I'm actually not sure what it's going to take to re-write the debian packaging stuff to work "correctly" with the new hatch build?

I do know a bit about debian packaging so I might be able to help out what that. The pep-517 backend stuff will be new to me, but I assume the upstream debian python packaging helper scripts know about it.

@moto-timo
Copy link
Collaborator

Seems like debian/ is ok after a quick review (I didn't try to build). packaging/bmaptool.spec is currently python2 only and will need some updates for modern Fedora and similar distributions. I am a Fedora packager, so I should be able to help.

@moto-timo
Copy link
Collaborator

Upstream Fedora will eventually need to switch over to our new repo, but the current source for the old bmap-tools.spec is https://src.fedoraproject.org/rpms/bmap-tools/blob/rawhide/f/bmap-tools.spec

@twoerner
Copy link
Collaborator

Okay, I'll merge this.
But perhaps keep in mind that more needs to be done wrt debian and fedora packaging.

@twoerner twoerner merged commit 710b29d into yoctoproject:main Mar 26, 2024
6 checks passed
@twoerner
Copy link
Collaborator

hmm.... i don't think six is an optional dependency.

using a clean update/install it fails with:

$ bmaptool --version
Traceback (most recent call last):
  File "/2opt/devel/bmaptool/venv.python3.11/bin/bmaptool", line 5, in <module>
    from bmaptool.CLI import main
  File "/2opt/devel/bmaptool/venv.python3.11/lib64/python3.11/site-packages/bmaptool/CLI.py", line 42, in <module>
    from . import BmapCreate, BmapCopy, BmapHelpers, TransRead
  File "/2opt/devel/bmaptool/venv.python3.11/lib64/python3.11/site-packages/bmaptool/TransRead.py", line 36, in <module>
    from six.moves.urllib import parse as urlparse
ModuleNotFoundError: No module named 'six'

@JPEWdev
Copy link
Collaborator Author

JPEWdev commented Mar 26, 2024

Blerg sorry

@moto-timo
Copy link
Collaborator

moto-timo commented Mar 26, 2024

six shouldn't need to be required, as the entire purpose is 2 to 3 transition. We probably can refactor that TransReader to not conditionally need to support python2 anymore. But easier said than done perhaps.

@moto-timo
Copy link
Collaborator

We may need to refactor for a newer urllib (urllib3 or something like that from memory).

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.

3 participants