-
Notifications
You must be signed in to change notification settings - Fork 1
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
NH-14860 Fix sdist and wheel creation #24
NH-14860 Fix sdist and wheel creation #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @tammy-baylis-swi! I tried a pip install <generated sdist>
on my little test stack and appears to all work! Example trace: https://my.dc-01.dev-ssp.solarwinds.com/138094000386806784/traces/BE56E52C856D6CF155A9893529112C50/BF978382E95F9E65/details
Left a few minor comments which can be addressed in follow-on PR.
@@ -49,52 +56,49 @@ def is_alpine_distro(): | |||
return False | |||
|
|||
|
|||
def python_version_supported(): | |||
if sys.version_info[0] == 3 and sys.version_info[1] > 3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the second (minor?) version check be 5 since NH python only supports 3.6+?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct! This is a remnant from AO I forgot about. Will fix.
Speaking of! OTel Python may stop supporting 3.6, if this gets approved and released: open-telemetry/opentelemetry-python#2763 That's for us later though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find, I'm glad you're keeping eyes on the new OTel happenings :)
|
||
if not (python_version_supported() and os_supported()): | ||
logger.warn( | ||
"[SETUP] This package supports only Python 3.5 and above on Linux. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit Python 3.6 and above
|
||
logger.info("Create links to platform specific liboboe library file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor but I don't get these messages anymore when running pip install -v
, maybe distutils.log
was somehow able to know when pip verbose logging was specified while the (setuptools recommended) standard logging doesn't. It's not a blocker for this PR but would be good to know how to enable this for future troubleshooting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think you're right. logging
has limited reach. I'll take a look.
Thanks @cheempz ! Yep, I'll merge this then address the minor suggestions in next PR. |
This PR covers some of the changes required for NH Python packaging and distribution. I will be switching from
setup.py
topyproject.toml
use in a separate PR.Summary:
ImportError
frommake sdist
, probably becausesetup.py
was missingCustomBuild
helper method.make manylinux-wheels
, usingmanylinux2014_x86_64
PyPA image (same as AO).distutils
tosetuptools
usage.decorator
andsix
dependencies.As we chatted separately, this removes the
setup.cfg
and puts more responsibility insetup.py
. It’s not the best solution since it’s now a biggersetup.py
than before and I think I read somewhere that the preferred pattern is a loadedsetup.cfg
while allsetup.py
does is make thesetup()
call. But a switch topyproject.toml
surpasses importance of doing a cleanup of this so I'm leaving it this way for now.Quick test doc outlining how NH agent
make sdist
works and is no longer full of extra fluff:https://swicloud.atlassian.net/wiki/spaces/NIT/pages/3138683446/2022-06-30+sdist+generation
make manylinux-wheels
ends inDone.
after creating and auditing wheels in build container e.g.