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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilise fetchTree #10068

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

Stabilise fetchTree #10068

wants to merge 5 commits into from

Conversation

thufschmitt
Copy link
Member

@thufschmitt thufschmitt commented Feb 23, 2024

Motivation

Stabilise the core of fetchTree.

This includes:

  • the fetchTree function itself
  • all the existing fetchers, except git (still unstable because of some reproducibility concerns)

Context

Agreed-upon to be stabilised after the 2.20. About time we do it now :)

This introduces two new experimental features (yay!) for the git fetcher and flakeref-style urls which are still experimental.

Priorities and Process

Add 馃憤 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Add two new features:
- `fetch-tree-git` to enable the `git` fetcher for `fetchTree`
- `fetch-tree-urls` to enable the URL-like syntax for `fetchTree`

`fetch-tree` is kept as a backwards-compatibility alias for the two new
features.
Run the fetchTree-file test without the experimental feature to make
sure that it works properly
@github-actions github-actions bot added with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world labels Feb 23, 2024
@thufschmitt thufschmitt marked this pull request as ready for review February 23, 2024 14:20
Make sure that enabling `fetch-tree` also enables `fetch-tree-git` and
`fetch-tree-urls`.
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.

Most fetchers still have problems that we must not commit to.

The core of `fetchTree` is now stable.
This includes
- the `fetchTree` function itself
- all the existing fetchers, except `git` (still unstable because of some reproducibility concerns)
Copy link
Member

Choose a reason for hiding this comment

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

git is the only one we've actually improved during this time.

gitlab has known problems (wrong field names, wrong url syntax)

mercurial and all the other fetchers we haven't talked about are under-used and should be assumed to have repro problems

github is still git archive-based, not tree-based.

  • users who switch between git and github fetchers will be surprised to see their hash changed
  • git can't be reliably be used as a fallback implementation for github fetching because of export-subst (export-ignore might work)

tarball is probably the only fetcher without severe issues and also been reviewed properly. All the other ones should remain feature gated.

@thufschmitt
Copy link
Member Author

Discussed during the Nix maintainers meeting on 2024-02-26.
Approved, but needs to be more restrictive, and blocked on #10115

  • Stabilizing everything but git was too eager, we need to just stabilize the tarball one
  • Should it return a path value as the output path?
    • Lazy trees require that
    • We can also just keep fetchTree not lazy
  • Any reason not to return a path?
    • Will cause errors in some usages
  • Any complexity in making it return a path?

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-02-28-nix-team-meeting-129/40499/1

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 with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: Defined work
Development

Successfully merging this pull request may close these issues.

None yet

3 participants