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

NH-35046 No longer need PLATFORM env var for sdist installation, tests #119

Merged
merged 12 commits into from
Mar 8, 2023

Conversation

tammy-baylis-swi
Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi commented Mar 7, 2023

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:

  1. Change this line in testbed to 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.
  2. Build test app without setting ENV for platform: docker-compose -f docker-compose.flask-from-testpypi.yml build --no-cache
  3. No longer gets pip error at build

Install tests pass after removing PLATFORM from verify_install.yaml: https://github.com/solarwindscloud/solarwinds-apm-python/actions/runs/4359203186

I can run the testbed without PLATFORM now; changes in this PR.

PLATFORM is optional for some make actions (Makefile): built wheel tagging (in GH workflows for releases or local make package) and lambda builds. If not provided, PLATFORM defaults to x86_64. PLATFORM=aarch64 is provided by the build_publish_aarch64 GH actions that build aarch64 on QEMU (where uname -m doesn't seem to be aarch64).

This also makes the --no-binary / --only-binary pip calls in the install tests less ambiguous/more correct.

@tammy-baylis-swi tammy-baylis-swi changed the title Rm PLATFORM env var from builds,tests NH-35046 No longer need PLATFORM env var for sdist installation, tests Mar 7, 2023
@tammy-baylis-swi tammy-baylis-swi marked this pull request as ready for review March 7, 2023 22:48
@tammy-baylis-swi tammy-baylis-swi requested a review from a team March 7, 2023 22:48
Copy link
Contributor

@cheempz cheempz left a 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)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@tammy-baylis-swi
Copy link
Contributor Author

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 😃

Copy link
Contributor

@cheempz cheempz left a 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!

@tammy-baylis-swi tammy-baylis-swi merged commit 125d750 into main Mar 8, 2023
@tammy-baylis-swi tammy-baylis-swi deleted the NH-35046-adjust-platform-var-usage branch March 8, 2023 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants