-
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-35046 No longer need PLATFORM env var for sdist installation, tests #119
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 pending minor question, thanks for addressing this so quickly @tammy-baylis-swi!
@@ -101,7 +91,7 @@ function get_wheel(){ | |||
exit 1 | |||
fi | |||
else | |||
pip_options=(--only-binary solarwinds-apm --dest "$wheel_dir") | |||
pip_options=(--only-binary solarwinds-apm --dest "$wheel_dir" solarwinds-apm) |
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 is the addition of solarwinds-apm
needed? Seeing later that we do seem to append it to pip_options.
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.
I think it is, technically. pip --only-binary
and --no-binary
are supposed to have a <format_control>
value provided: https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-only-binary
The way it's currently written in the install tests still works and I'm not sure why given the above. The install tests pass on GH (have been for a long time) and this also passes on my local on main
branch: MODE=testpypi docker-compose run --rm py3.7-install-alpine3.12
.
The missing <format_control>
becomes an issue when I try to install from the testbed for flask-from-testpypi
. Both of these error:
pip install --no-binary solarwinds-apm==0.8.0
pip install --no-binary solarwinds-apm
But this is ok:
pip install --no-binary solarwinds-apm==0.8.0 solarwinds-apm
Actually this is failing too...
pip install --no-binary solarwinds-apm solarwinds-apm
...Ok. I'm going to revert b7fcfd6 and do a fresh publish and test.
This reverts commit b7fcfd6.
Thanks for first review @cheempz ! I've reverted b7fcfd6, published to TestPyPI as 0.8.1.5, and run the install tests again which seem ok: https://github.com/solarwindscloud/solarwinds-apm-python/actions/runs/4369183330 Please could you have another look and let me know if anything else needed 😃 |
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 for the revisit @tammy-baylis-swi!
Fixes an issue where
PLATFORM
environment variable is required by customers(!) to install Python APM library from sdist, i.e. on Alpine Linux. Thank you @cheempz for spotting this.PLATFORM
is no longer mandatory for extension creation by setuptools (setup.py) nor testing. I published this as TestPyPI version 0.8.1.4 and I tested installation of this with:RUN pip install --extra-index-url https://test.pypi.org/simple/ --no-binary solarwinds-apm solarwinds-apm==0.8.1.4
to force installation from sdist instead of wheels.ENV
for platform:docker-compose -f docker-compose.flask-from-testpypi.yml build --no-cache
Install tests pass after removing
PLATFORM
from verify_install.yaml: https://github.com/solarwindscloud/solarwinds-apm-python/actions/runs/4359203186I can run the testbed without
PLATFORM
now; changes in this PR.PLATFORM
is optional for somemake
actions (Makefile): built wheel tagging (in GH workflows for releases or localmake package
) and lambda builds. If not provided,PLATFORM
defaults tox86_64
.PLATFORM=aarch64
is provided by thebuild_publish_aarch64
GH actions that build aarch64 on QEMU (whereuname -m
doesn't seem to beaarch64
).This also makes the
--no-binary
/--only-binary
pip calls in the install tests less ambiguous/more correct.