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

Fetch remote tarballs/zips using nix-prefetch-url --unpack #585

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pwm
Copy link

@pwm pwm commented Nov 8, 2022

Fetching remote tarballs/zips is common enough of a use-case that warrants treating them specially by bypassing other fetching options and going for nix-prefetch-url --unpack directly. This speeds up large tarballs as cabal2nix currently downloads them twice, where the first fetch assumes an unpacked url and thus fails.

Fetching `tar.gz` files is common enough of a use-case that warrants
treating them specially by bypassing other fetching options and using
`nix-prefetch-url --unpack` directly. This speeds up large tarballs as
`cabal2nix` currently downloads them twice as the first fetch assumes an
unpacked url and thus will fail.
@pwm pwm force-pushed the pwm/tarball-fetch branch from ccedf3d to 34fbe05 Compare November 9, 2022 15:58
@pwm pwm changed the title Fetch .tar.gz using nix-prefetch-url --unpack Fetch remote tarballs/zips using nix-prefetch-url --unpack Nov 9, 2022
Copy link

@dschrempf dschrempf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a regular cabal2nix contributor, but I saw your discussion on the matrix channel. Here are my two cents:

I think the fetchPackedUrl function needs a top level comment explaining why it was added.

Aslo, I think the process function should be top-leveled and reused (by fetch and fetchPackedUrl).

@pwm
Copy link
Author

pwm commented Nov 10, 2022

@dschrempf thanks for the feedback

I think the fetchPackedUrl function needs a top level comment explaining why it was added.

Agreed, will add.

Aslo, I think the process function should be top-leveled and reused (by fetch and fetchPackedUrl).

I actually did that first but then thought it might be better to try and minimise the change which is why i did it this way. Happy to lift it out (I think it makes sense) but that will mean a slightly bigger PR (not sure if that's an issue though).

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