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

Add a 'download' language for fetching pre-built binaries #3020

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fangfufu
Copy link
Contributor

@fangfufu fangfufu commented Oct 6, 2023

This commit implements #1453 (comment)

@asottile
Copy link
Member

asottile commented Oct 6, 2023

please no pathlib, dataclasses, or backslashes -- really you probably don't need any classes at all. I'm also not sure why the strange sha256 -- use hexdigest which is way more standardized.

there's a lot of code here and almost zero tests so it's not in a reviewable state -- please convert your PR to draft until you're ready for a first round

@fangfufu
Copy link
Contributor Author

fangfufu commented Oct 6, 2023

What do you mean by

you probably don't need any classes at all

The " the strange sha256" is subresource integrity.

What do you mean by "no backslashes"?

@asottile
Copy link
Member

asottile commented Oct 6, 2023

What do you mean by

you probably don't need any classes at all

your code has a bunch of classes with nontrivial methods -- simple plain-old-data classes is preferred -- check the approaches used by most of the rest of the codebase

The " the strange sha256" is subresource integrity.

just because it is standardized doesn't mean it is good -- we aren't optimizing for http headers where characters are at a minimum so use normal hexdigest which is far more common and much easier to compute

What do you mean by "no backslashes"?

your source code contains many very very ugly backslashes

@fangfufu
Copy link
Contributor Author

fangfufu commented Oct 8, 2023

so use normal hexdigest which is far more common

@asottile, normal checksum, i.e. the output from sha256sum or md5sum are not in hex. If we use hexdigest here, then you also need something to generate those hexdigest, right?

@asottile
Copy link
Member

asottile commented Oct 8, 2023

so use normal hexdigest which is far more common

@asottile, normal checksum, i.e. the output from sha256sum or md5sum are not in hex. If we use hexdigest here, then you also need something to generate those hexdigest, right?

what do you mean yes they are

@fangfufu
Copy link
Contributor Author

fangfufu commented Oct 8, 2023

Ah never mind, for some reason I thought md5sum and sha256sum outputs string in base64 format.

@asottile
Copy link
Member

I recently build something similar that has a format I was pretty happy with -- might be able to use that format or similar here: https://github.com/asottile/setuptools-download

@fangfufu
Copy link
Contributor Author

@asottile , do you have a preferred way of handling the different OS and architecture?

@asottile
Copy link
Member

setuptools-download handles that with markers -- essentially filters on sys.platform, platform.machine() and os.name (though os.name is perhaps redundant)

@DrummyFloyd

This comment was marked as off-topic.

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.

None yet

3 participants