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

fetchTree: handle tarballs with multiple top-level files #9053

Closed

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Sep 27, 2023

Motivation

See #7083. While the issue can be worked around by using the file type, it imposes a dependency on nixpkgs and makes implementation of other fetchers based on downloadTarball more difficult.

Context

Issue #7083 suggests two solutions to this problem:

  1. If there are multiple top-level files, use the archive root
  2. A stripRoot attribute, like in nixpkgs' fetchzip

This patch implements (1). It is forward-compatible with (2) if a missing stripRoot is interpreted as "automatic".

Fixes #7083

Priorities

Add 👍 to pull requests you find important.

@fgaz fgaz requested a review from edolstra as a code owner September 27, 2023 11:07
@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Sep 27, 2023
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Needs a test in tests/tarball.sh

@fgaz fgaz force-pushed the libfetchers-tarball-multiple-toplevel branch from 001c3fb to 3957e71 Compare September 28, 2023 07:05
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Sep 28, 2023
Issue NixOS#7083 suggests two solutions to this problem:

1. If there are multiple top-level files, use the archive root
2. A stripRoot attribute, like in nixpkgs' fetchzip

This patch implements (1). It is forward-compatible with (2) if
a missing stripRoot is interpreted as "automatic".

Fixes NixOS#7083
@fgaz fgaz force-pushed the libfetchers-tarball-multiple-toplevel branch from 3957e71 to bfd1ba8 Compare September 28, 2023 07:42
@fgaz fgaz requested a review from roberth September 28, 2023 08:24
@edolstra
Copy link
Member

I'd prefer not to do this. The lazy-trees branch completely changes how tarballs are handled (they're imported into a local git cache and accessed from there), and it's not clear how to handle tarballs with more than one root object.

@fgaz
Copy link
Member Author

fgaz commented Oct 25, 2023

@edolstra Why isn't it clear? Is it because we don't know whether to strip the root until all entries are processed? If so, what about a stripRoot parameter?

@thufschmitt
Copy link
Member

what about a stripRoot parameter?

@fgaz That seems to be a good solution (defaulting it to true and mentioning it in the error message if there's more than one toplevel dir)

@roberth
Copy link
Member

roberth commented Nov 24, 2023

and it's not clear how to handle tarballs with more than one root object.

libgit should make this quite easy. Assuming you want to have a streaming implementation, you stream all the contents into blobs, and keep all directory entries, in the original tarball layout at first (probably in memory, because maintaining such a data structure on disk would be inefficient - could be done though, don't care). At the end, you check whether the raw root contains a single directory. If it does, you return its contents instead of the root.
Note that in both tarball and zip you have to read at least the archive end to determine the raw root contents, so streaming is only about minimizing memory usage, not speeding up access.

what about a stripRoot parameter?

I'm not so sure about that. Less "configuration" is generally better.

@fricklerhandwerk
Copy link
Contributor

Triaged in Nix team meeting:

  • @edolstra: could merge this part of lazy trees and collaborate with @fgaz on getting that feature into it
    • can do it even simpler, and always create a directory, and if that ends up only containing a directory, throw away the excess layer
  • @roberth: getting more of that WIP merged would generally be great
  • decision: do it as discussed

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-11-24-nix-team-meeting-minutes-106/35955/1

@mrtnvgr
Copy link

mrtnvgr commented Feb 2, 2024

+1

@fgaz
Copy link
Member Author

fgaz commented Feb 2, 2024

I just noticed #9485 contains the lazy trees part that was mentioned by @fricklerhandwerk. @edolstra do you accept PRs targeting your branch? Should I wait until #9485 lands?

@pinpox
Copy link
Member

pinpox commented Mar 23, 2024

Are there any workarounds on how to handle zip flake inputs with mutiple files in the meantime?

@fgaz
Copy link
Member Author

fgaz commented Mar 26, 2024

@pinpox You could fetch as file and import from derivation.


#9485 was merged, so I guess this is unblocked?

can do it even simpler, and always create a directory, and if that ends up only containing a directory, throw away the excess layer

@pinpox
Copy link
Member

pinpox commented Mar 27, 2024

@pinpox You could fetch as file and import from derivation.

Hm, true, but has the downside of having to provide and update the hash myself instead of it being managed in flake.lock. I think this is still very relevant and an important feature.

@thufschmitt
Copy link
Member

has the downside of having to provide and update the hash myself instead of it being managed in flake.lock

You can still have it as a flake input using the file input type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetching Networking with the outside (non-Nix) world, input locking with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Flake: cannot extract input tarball / zip that has more than 1 top-level directory
8 participants