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

feat(binaries): improve platform and architecture support #233

Closed
wants to merge 64 commits into from
Closed

feat(binaries): improve platform and architecture support #233

wants to merge 64 commits into from

Conversation

HKGx
Copy link

@HKGx HKGx commented Jan 19, 2022

Change Summary

Resolves properly #195
Pretty much just porting https://github.com/prisma/engines-wrapper to Python.

Checklist

  • Unit tests for the changes exist
  • Tests pass without significant drop in coverage
  • Documentation reflects changes where applicable
  • Test snapshots have been updated if applicable

Agreement

By submitting this pull request, I confirm that you can use, modify, copy and redistribute this contribution, under the terms of your choice.

@HKGx HKGx marked this pull request as ready for review January 19, 2022 22:48
@HKGx HKGx marked this pull request as draft January 19, 2022 22:49
@RobertCraigie
Copy link
Owner

RobertCraigie commented Jan 19, 2022

Thank you! This is a much needed improvement.

Could you please add unit tests to ensure that all possible URLs that we generate will not result in a 404. You could base it off of this script (but should probably parallelise the requests with the AsyncClient)

import httpx
from prisma.binaries import ENGINES

url = ENGINES[0].url
print(url)

client = httpx.Client()
response = client.head(url)
response.raise_for_status()

client.close()

needs cleaning up and tests
@HKGx
Copy link
Author

HKGx commented Jan 20, 2022

Could you please add unit tests to ensure that all possible URLs that we generate will not result in a 404. You could base it off of this script (but should probably parallelise the requests with the AsyncClient)

@RobertCraigie will do it later. Just pushed the basic implementation and now I'm going to sleep. It should be on par with the default TypeScript one.
Tested it on Windows. Wanted to test on Debian but Prisma is not built for the newest OpenSSL 3.0 😩

@RobertCraigie
Copy link
Owner

RobertCraigie commented Jan 20, 2022

@HKGx No worries, thank you so much, it is much appreciated!

Wanted to test on Debian but Prisma is not built for the newest OpenSSL 3.0 😩

I hadn't even realised there was an OpenSSL 3.0 to be honest, is it worth making an issue in https://github.com/prisma/prisma-engines?

@RobertCraigie
Copy link
Owner

RobertCraigie commented Jan 20, 2022

@HKGx I've pushed some minor changes and small fixes.

Some thoughts:

  • I'm happy with the name change of PRISMA_CLI_URL to PRISMA_CLI_MIRROR and PRISMA_ENGINES_MIRROR but we need to document that somewhere (even if it wasn't documented before) as it is a breaking change.
  • Does your current implementation respect local binary paths? (i.e. binaries in the working directory)

Regarding code style:

  • I prefer single quotes 'a' over double quotes "b"
  • I generally think if you need to create a method like: engine_url_for then that probably belongs within a class

Great work so far!

@RobertCraigie RobertCraigie added this to the v0.5.0 milestone Jan 20, 2022
@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #233 (21f3da8) into main (7f70252) will decrease coverage by 57.70%.
The diff coverage is 48.41%.

❗ Current head 21f3da8 differs from pull request most recent head 6f594bf. Consider uploading reports for the commit 6f594bf to get more accurate results

@@             Coverage Diff             @@
##             main     #233       +/-   ##
===========================================
- Coverage   96.52%   38.81%   -57.71%     
===========================================
  Files         113      102       -11     
  Lines        5637     5531      -106     
  Branches      324      330        +6     
===========================================
- Hits         5441     2147     -3294     
- Misses        147     3304     +3157     
- Partials       49       80       +31     
Impacted Files Coverage Δ
src/prisma/_compat.py 77.77% <0.00%> (-22.23%) ⬇️
src/prisma/binaries/__init__.py 100.00% <ø> (ø)
src/prisma/cli/commands/version.py 0.00% <ø> (-95.24%) ⬇️
src/prisma/engine/errors.py 78.57% <ø> (-11.43%) ⬇️
src/prisma/engine/utils.py 38.15% <ø> (-55.27%) ⬇️
tests/test_binaries.py 0.00% <0.00%> (-100.00%) ⬇️
tests/test_cli/test_fetch.py 0.00% <0.00%> (-100.00%) ⬇️
tests/test_engine.py 0.00% <0.00%> (-100.00%) ⬇️
src/prisma/binaries/platform.py 55.84% <51.51%> (-14.37%) ⬇️
src/prisma/binaries/binaries.py 78.89% <77.08%> (-12.28%) ⬇️
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f70252...6f594bf. Read the comment docs.

@HKGx HKGx marked this pull request as ready for review January 27, 2022 22:15
@RobertCraigie RobertCraigie modified the milestones: v0.5.0, v0.6.0 Feb 1, 2022
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.

2 participants