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

refactor: remove python 3.8 support (FIxes #4579) #4654

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

vedpawar2254
Copy link
Contributor

FIxes #4579 Removed python 3.8 support

Removed the accidentally checked in filed that I added when i did git add . and worked on other feedbacks

  • remove any package workarounds that reference python 3.8 (there's some in dev-requirements.txt for sure)
  • change configuration for pyupgrade in .pre-commit-config.yaml
  • look through code to see if we have other python 3.8 specific workarounds that should be flagged for eventual [x] removal (can set up separate issues for these if we don't want to pull them out right away). There might be a couple in the tests specifically.

@terriko terriko changed the title FIxes #4579 choreFIxes #4579 Dec 27, 2024
@terriko terriko changed the title choreFIxes #4579 refactor: remove python 3.8 support (FIxes #4579) Dec 27, 2024
@terriko
Copy link
Contributor

terriko commented Dec 27, 2024

I forgot to change the title before letting the linters run, so gitlint will complain but there's nothing more you need to do there. Thank you so much for working on this!

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

Looks like tests are failing, see comment on the file that seems to be causing the problem.

cve_bin_tool/vex_manager/parse.py Outdated Show resolved Hide resolved
@terriko
Copy link
Contributor

terriko commented Dec 30, 2024

Thank you for the update! I've approved the tests to run again.

PS - I'm going to be out the next two days celebrating new years, so if things get slow this week that's why!

@vedpawar2254
Copy link
Contributor Author

No worries, i'll probably be out too, wishing you an early happy new year

Copy link
Contributor

@Molkree Molkree left a comment

Choose a reason for hiding this comment

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

Cool stuff, came to this repo to check if cve-bin-tool dropped 3.8 support and it's nice that someone already started working on this.

Tests seem to be failing due to incorrect import as noted in one of my comments, hopefully it's the only major issue.

raise OSError(
"Python no longer provides security updates for version 3.7 as of June 2023. Please upgrade to python 3.8+ to use CVE Binary Tool."
"Python no longer provides security updates for version 3.7 as of June 2023. Please upgrade to python 3.9+ to use CVE Binary Tool."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Python no longer provides security updates for version 3.7 as of June 2023. Please upgrade to python 3.9+ to use CVE Binary Tool."
"Python no longer provides security updates for version 3.8 as of October 2024. Please upgrade to Python 3.9+ to use CVE Binary Tool."

raise OSError(
"Python no longer provides security updates for version 3.7 as of June 2023. Please upgrade to python 3.8+ to use CVE Binary Tool."
"Python no longer provides security updates for version 3.7 as of June 2023. Please upgrade to python 3.9+ to use CVE Binary Tool."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Python no longer provides security updates for version 3.7 as of June 2023. Please upgrade to python 3.9+ to use CVE Binary Tool."
"Python no longer provides security updates for version 3.8 as of October 2024. Please upgrade to Python 3.9+ to use CVE Binary Tool."

@@ -7,7 +7,7 @@
from logging import Logger
from pathlib import Path
from string import ascii_lowercase
from typing import DefaultDict, Dict, List
from typing import DefaultDict
Copy link
Contributor

Choose a reason for hiding this comment

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

typing.DefaultDict is also deprecated since 3.9
We should use collections.defaultdict instead

from enum import Enum
from pathlib import Path
from typing import DefaultDict, Iterator, List, NamedTuple, Pattern, Set, Union
from re import Pattern
from typing import DefaultDict, NamedTuple, Union
Copy link
Contributor

Choose a reason for hiding this comment

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

should be collections.defaultdict

@@ -1,14 +1,15 @@
# Copyright (C) 2024 Intel Corporation
# SPDX-License-Identifier: GPL-3.0-or-later

from typing import Any, DefaultDict, Dict, Set, Union
from collections.abc import DefaultDict
Copy link
Contributor

Choose a reason for hiding this comment

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

This import is incorrect, collections.abc doesn't have DefaultDict in it.
It should be collections.defaultdict

How did this error happen? Were these replacements done manually? It should be automated by pyupgrade

gitlint==v0.19.1
interrogate
mypy==v1.13.0
pytest>=7.2.0
pytest-xdist
pytest-cov
pytest-asyncio

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +78 to +79
virtualenv -p python3.12 venv3.8
virtualenv -p python3.12 venv3.9
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea was to show how to create different environments for different Python versions, so we should preserve that (and keep correct naming)

Suggested change
virtualenv -p python3.12 venv3.8
virtualenv -p python3.12 venv3.9
virtualenv -p python3.11 venv3.11
virtualenv -p python3.12 venv3.12

At the same time I don't even see Python 3.12 in the list of supported versions on PyPI. What's the policy these days, @terriko?

Copy link
Contributor

Choose a reason for hiding this comment

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

The next version should support 3.9-3.12 but not yet supporting 3.13 (see #4663 ; it should be coming soon-ish). I think 3.4 was supposed to support 3.12 but I may have missed updating the setup file before building the package, so we'll turn it on for 3.4.1.

virtualenv -p python3.8 venv3.8
virtualenv -p python3.9 venv3.9
virtualenv -p python3.12 venv3.8
virtualenv -p python3.12 venv3.9
```

To activate one of these (the example uses 3.8), run the tests, and deactivate:
Copy link
Contributor

Choose a reason for hiding this comment

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

I see more mentions of 3.8 here and in other places that should probably be removed.

@@ -92,7 +92,7 @@ deactivate

- You can see the code for scanner tests in ['test/test_scanner.py'](https://github.com/intel/cve-bin-tool/blob/main/test/test_scanner.py)
- You can see checker wise test data in ['test/test_data'](https://github.com/intel/cve-bin-tool/blob/main/test/test_data)
- If you just want to add a new mapping test for a checker, add a dictionary of _product_, _version_ and _version_strings_ in the mapping_test_data list . Here, _version_strings_ are the list of strings that contain version signature or strings that commonly can be found in the module. For example: this is how the current mapping_test_data for gnutls look like. You should add the details of the new test case data at the end of `mapping_test_data` list:
- If you just want to add a new mapping test for a checker, add a dictionary of _product_, _version_ and _version_strings_ in the mapping*test_data list . Here, \_version_strings* are the list of strings that contain version signature or strings that commonly can be found in the module. For example: this is how the current mapping_test_data for gnutls look like. You should add the details of the new test case data at the end of `mapping_test_data` list:
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like accidental change

Copy link
Contributor

Choose a reason for hiding this comment

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

I see a lot of changes in this file that are not related to the 3.8 removal

  1. Whitespace changes: mostly removing double whitespace in paragraphs, that are not paragraph breaks in Markdown. There are no changes to layout as far as I can see.
  2. Changes in links in the index at the start: everything still works (at least in GitHub UI). I'm curious why it was with escapes before. Did we check if the links still work in VSC? Note: one link that was touched is broken but it's broken in main as well. It's #--vex-output, should be #--vex-output-vex-file I guess (not checked). It can be fixed in this PR IMO.

Outside the scope of this PR:

  1. a bit weird that it's --vex-output VEX_FILE but --sbom-output SBOM_OUTPUT, inconsistent naming.
  2. Ordering in the index is incorrect at times: in manual it's (1) vex-output, (2) vex-type, (3) sbom-output, (4) sbom-type, (5) sbom-format but in index it's 3, 4, 5, 2, 1. Maybe more!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your help @Molkree, I really appreciate it 👍, I'll look into the changes you requested

Comment on lines +244 to +300

| | | | Available checkers | | | |
| --------------- | ------------- | ------------------ | ------------------ | ---------------- | --------------- | ------------- |
| accountsservice | acpid | apache_http_server | apcupsd | apparmor | asn1c | assimp |
| asterisk | atftp | avahi | axel | bash | bind | binutils |
| bird | bison | bluez | boinc | botan | bro | bubblewrap |
| busybox | bwm_ng | bzip2 | c_ares | capnproto | ceph | chess |
| chrony | civetweb | clamav | collectd | commons_compress | connman | coreutils |
| cpio | cpp_httplib | cronie | cryptsetup | cups | curl | cvs |
| darkhttpd | dav1d | davfs2 | dbus | debianutils | dhclient | dhcpcd |
| dhcpd | dlt_daemon | dmidecode | dnsmasq | docker | domoticz | dosfstools |
| dotnet | dovecot | doxygen | dpkg | dropbear | e2fsprogs | ed |
| elfutils | emacs | enscript | exfatprogs | exim | exiv2 | f2fs_tools |
| faad2 | fastd | ffmpeg | file | firefox | flac | fluidsynth |
| freeradius | freerdp | fribidi | frr | gawk | gcc | gdal |
| gdb | gdk_pixbuf | ghostscript | gimp | git | glib | glibc |
| gmp | gnomeshell | gnupg | gnutls | go | gpgme | gpsd |
| graphicsmagick | grep | grub2 | gstreamer | gupnp | gvfs | gzip |
| haproxy | harfbuzz | haserl | hdf5 | heimdal | hostapd | hunspell |
| hwloc | i2pd | icecast | icu | iperf3 | ipmitool | ipsec_tools |
| iptables | irssi | iucode_tool | iwd | jack2 | jacksondatabind | janus |
| jasper | jhead | jq | json_c | kbd | keepalived | kerberos |
| kexectools | kodi | kubernetes | ldns | lftp | libarchive | libass |
| libbpg | libcoap | libconfuse | libcurl | libdb | libde265 | libebml |
| libevent | libexpat | libgcrypt | libgd | libgit2 | libheif | libical |
| libidn2 | libinput | libjpeg | libjpeg_turbo | libksba | liblas | libmatroska |
| libmemcached | libmicrohttpd | libmodbus | libnss | libopenmpt | libpcap | libraw |
| librsvg | librsync | libsamplerate | libseccomp | libsndfile | libsolv | libsoup |
| libsrtp | libssh | libssh2 | libtasn1 | libtiff | libtomcrypt | libupnp |
| libuv | libvips | libvirt | libvncserver | libvorbis | libvpx | libxslt |
| libyaml | lighttpd | linux_kernel | linuxptp | lldpd | logrotate | lrzip |
| lua | luajit | lxc | lynx | lz4 | mailx | mariadb |
| mbedtls | mdadm | memcached | micropython | minetest | mini_httpd | minicom |
| minidlna | miniupnpc | miniupnpd | moby | modsecurity | monit | mosquitto |
| motion | mp4v2 | mpg123 | mpv | msmtp | mtr | mupdf |
| mutt | mysql | nano | nasm | nbd | ncurses | neon |
| nessus | netatalk | netdata | netkit_ftp | netpbm | nettle | nghttp2 |
| nginx | ngircd | nmap | node | ntfs_3g | ntp | ntpsec |
| oath_toolkit | open_iscsi | open_vm_tools | openafs | opencv | openjpeg | openldap |
| opensc | openssh | openssl | openswan | openvpn | orc | p7zip |
| pango | patch | pcre | pcre2 | pcsc_lite | perl | php |
| picocom | pigz | pixman | png | polarssl_fedora | poppler | postgresql |
| ppp | privoxy | procps_ng | proftpd | protobuf_c | pspp | pure_ftpd |
| putty | python | qemu | qpdf | qt | quagga | radare2 |
| radvd | raptor | rauc | rdesktop | readline | rpm | rsync |
| rsyslog | rtl_433 | rtmpdump | runc | rust | samba | sane_backends |
| sdl | seahorse | shadowsocks_libev | snapd | sngrep | snort | socat |
| sofia_sip | speex | spice | sqlite | squashfs | squid | sslh |
| stellarium | strongswan | stunnel | subversion | sudo | suricata | sylpheed |
| syslogng | sysstat | systemd | tar | tcpdump | tcpreplay | terminology |
| tesseract | thrift | thttpd | thunderbird | timescaledb | tinyproxy | tor |
| tpm2_tss | traceroute | transmission | trousers | ttyd | twonky_server | u_boot |
| udisks | unbound | unixodbc | upx | util_linux | varnish | vim |
| vlc | vorbis_tools | vsftpd | webkitgtk | wget | wireshark | wolfssl |
| wpa_supplicant | xerces | xml2 | xscreensaver | xwayland | yasm | zabbix |
| zchunk | zeek | zlib | znc | zsh | zstandard | |

Copy link
Contributor

@Molkree Molkree Jan 2, 2025

Choose a reason for hiding this comment

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

One more comment, this change should be reverted because this table is built automatically by a tool so it will be overwritten anyway
You can see an example here: #4648

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.

ci: remove python 3.8 support
4 participants