document `fetchTree` #9258

fricklerhandwerk · 2023-10-30T23:42:08Z

As discussed with @NixOS/nix-team.

Priorities

Add 👍 to pull requests you find important.

src/libexpr/primops/fetchTree.cc

tomberek · 2023-10-31T14:04:18Z

src/libexpr/primops/fetchTree.cc

+
+      <!-- TODO: It would be soooo much more predictable to work with (and
+      document) if `fetchTree` was a curried call with the first paramter for
+      `type` or an attribute like `builtins.fetchTree.git`! -->


Very interesting, nested builtin to disambiguation the type. On the other hand, this makes the idempotency a little more awkward.

src/libexpr/primops/fetchTree.cc

tomberek

A few fixups and TODOs, but this is in line with our last review session. Would be good to have a more discerning eye take a look (@infinisil , does this address your doctor concerns?)

Ericson2314 · 2023-10-31T14:29:46Z

This one is so big, I think it would be better to put in a separate file which is #included in.

infinisil

This looks so much better! Left some minor suggestions :)

src/libexpr/primops/fetchTree.cc

infinisil · 2023-10-31T17:35:02Z

src/libexpr/primops/fetchTree.cc

+      > **Example**
+      >
+      > Fetch a GitHub repository using the attribute set representation:
+      >
+      > ```nix
+      > builtins.fetchTree {
+      >   type = "github";


Better not use something that's experimental as an example.

There's a good chance we'll stabilize these together, considering that possibly a majority of usages of fetchTree are with this type.

edolstra · 2023-11-01T13:06:20Z

I'm not sure builtins.fetchTree is the best place to document this, especially since it now duplicates part of the "Types" section of the nix flakes page. Maybe we can have a separate page on fetchers and refer to that from both builtins.fetchTree and the nix flakes page?

fricklerhandwerk · 2023-11-01T13:39:08Z

Maybe we can have a separate page on fetchers and refer to that from both builtins.fetchTree and the nix flake page?

Given we're carving out this particular builtin, everything specific to its behavior should be part of its documentation.

The nix flake page can link to that, ans is quite large already anyway. We just didn't get as far as moving the information from there to here yet.

Ericson2314 · 2023-11-01T13:51:24Z

We do want things deduplicated long term, yes. But for now the only release blocker is worrying about the docs for the newly stable things. Deduplicating with the Flakes docs is not a release blocker because it comes later.

That said, my automation can help with this.

edolstra · 2023-11-03T13:35:02Z

Given we're carving out this particular builtin, everything specific to its behavior should be part of its documentation.

That makes no sense. There are a lot of builtins whose behaviour is not fully explained by the builtin's docs, e.g. anything having to do with the Nix store. It seems very poor writing to duplicate information everywhere.

infinisil · 2023-11-03T14:17:10Z

I agree with @edolstra that for reference docs, there shouldn't be duplication and a lot of linking seems preferable.

(though I'll take good docs with some duplication over bad docs any day of the week 🙂)

fricklerhandwerk · 2023-11-03T15:56:41Z

I never said we should duplicate information, I simply didn't get to moving it around in this PR yet. Although that may become quite an effort altogether, so don't count on it getting merged yesterday.

src/libexpr/primops/fetchTree.cc

roberth · 2023-11-06T12:12:07Z

src/libexpr/primops/fetchTree.cc

+      Downloads are cached in `$XDG_CACHE_HOME/nix`.
+      The remote source will be fetched from the network if:
+      - The resulting store path is not [valid](@docroot@/glossary.md#gloss-validity)
+      - There is no cache entry


and or or?

src/libexpr/primops/fetchTree.cc

nixos-discourse · 2023-11-09T16:20:38Z

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

https://discourse.nixos.org/t/2023-11-03-nix-team-meeting-minutes-100/35245/1

Co-authored-by: tomberek <tomberek@users.noreply.github.com> Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

we have to enable the new `fetchTree` experimental feature to render it at all. this was a bug introduced when adding that new feature flag.

fricklerhandwerk · 2023-12-04T15:09:40Z

Reworked together with @roberth. We decided to leave the documentation on the flakes man page as is, because it needs a more work to untangle. Also that page talks about flakerefs, which are more than just the fetchTree input attributes, so it's not really wrong to have that separate.

infinisil

Looking pretty good!

src/libexpr/primops/fetchTree.cc

Co-authored-by: Silvan Mosberger <github@infinisil.com>

roberth

Final fixups for now. This documentation is far from perfect, but it's the best we can do before overhauling it.

src/libexpr/primops/fetchTree.cc

roberth · 2023-12-10T04:39:24Z

Can't push to tweag. Please apply and auto-merge.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

DavHau · 2023-12-27T11:36:09Z

#9390 has been reverted by this PR accidentally I guess

This reverts commit 3c200da.

DavHau · 2023-12-27T12:14:17Z

#9390 has been reverted by this PR accidentally I guess

Sorry for the confusion. It hasn't been reverted.
But but now the git arguments are documented twice. One time in the fetchTree docs and another time in the fetchGit docs.

Both instances are incomplete and conflict each other. Few things I notice:

the description of shallow in fetchTree is wrong
some fields are missing from the fetchTree docs like: verifyCommit, publicKey, publicKeys, keytype
revCount is missing for fetchGit and it doesn't mention that the provided revCount will be checked against the computed revCount and that nix will fail if there is a mismatch. (TBH this feature seems useless, I'm not sure if it should be documented at all)

EDIT: Maybe it's worth considering deduplicating the docs. The docs of fetchGit could just point to fetchTree for the argument reference, or vice versa.

fricklerhandwerk requested review from tomberek, infinisil and a team October 30, 2023 23:42

fricklerhandwerk requested a review from edolstra as a code owner October 30, 2023 23:42

tomberek reviewed Oct 31, 2023

View reviewed changes

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

tomberek reviewed Oct 31, 2023

View reviewed changes

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

tomberek reviewed Oct 31, 2023

View reviewed changes

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

tomberek approved these changes Oct 31, 2023

View reviewed changes

infinisil suggested changes Oct 31, 2023

View reviewed changes

infinisil mentioned this pull request Nov 1, 2023

Poor fetchTree documentation #9249

Closed

2 tasks

roberth reviewed Nov 6, 2023

View reviewed changes

roberth mentioned this pull request Dec 4, 2023

Handle experimental feature dependencies correctly, completely, DRY #9532

Open

fricklerhandwerk force-pushed the doc-fetchTree branch from 405e3c4 to 398181e Compare December 4, 2023 15:07

github-actions bot added documentation new-cli Relating to the "nix" command labels Dec 4, 2023

fricklerhandwerk and others added 2 commits December 4, 2023 16:07

document fetchTree

f825180

Co-authored-by: tomberek <tomberek@users.noreply.github.com> Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

display experimental feature note at the top

7fcf764

we have to enable the new `fetchTree` experimental feature to render it at all. this was a bug introduced when adding that new feature flag.

fricklerhandwerk force-pushed the doc-fetchTree branch from 398181e to 7fcf764 Compare December 4, 2023 15:07

infinisil reviewed Dec 5, 2023

View reviewed changes

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

fix typo

fe569cf

Co-authored-by: Silvan Mosberger <github@infinisil.com>

fix typo

04b730b

Co-authored-by: Silvan Mosberger <github@infinisil.com>

roberth approved these changes Dec 10, 2023

View reviewed changes

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

fricklerhandwerk and others added 2 commits December 10, 2023 05:55

fix typos

cf9101e

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

clarify git revision

4c5465f

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

fricklerhandwerk enabled auto-merge (squash) December 10, 2023 05:00

fricklerhandwerk merged commit 3c200da into NixOS:master Dec 10, 2023
8 checks passed

pwaller mentioned this pull request Dec 10, 2023

Flake inputs fetched and unpacked despite outputs being a cache hit #9570

Open

fricklerhandwerk added this to the fetchTree stabilisation milestone Dec 18, 2023

DavHau added a commit to DavHau/nix that referenced this pull request Dec 27, 2023

Revert "document fetchTree (NixOS#9258)"

e2ee9a0

This reverts commit 3c200da.

fricklerhandwerk mentioned this pull request Jan 7, 2024

update docs on fetchGit shallow clone behavior #9704

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

document `fetchTree` #9258

document `fetchTree` #9258

fricklerhandwerk commented Oct 30, 2023 •

edited

Loading

tomberek Oct 31, 2023 •

edited

Loading

tomberek left a comment

Ericson2314 commented Oct 31, 2023

infinisil left a comment

infinisil Oct 31, 2023

roberth Dec 5, 2023

edolstra commented Nov 1, 2023

fricklerhandwerk commented Nov 1, 2023

Ericson2314 commented Nov 1, 2023

edolstra commented Nov 3, 2023

infinisil commented Nov 3, 2023

fricklerhandwerk commented Nov 3, 2023

roberth Nov 6, 2023

nixos-discourse commented Nov 9, 2023

fricklerhandwerk commented Dec 4, 2023

infinisil left a comment

roberth left a comment

roberth commented Dec 10, 2023

DavHau commented Dec 27, 2023

DavHau commented Dec 27, 2023 •

edited

Loading

document fetchTree #9258

document fetchTree #9258

Conversation

fricklerhandwerk commented Oct 30, 2023 • edited Loading

Priorities

tomberek Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

tomberek left a comment

Choose a reason for hiding this comment

Ericson2314 commented Oct 31, 2023

infinisil left a comment

Choose a reason for hiding this comment

infinisil Oct 31, 2023

Choose a reason for hiding this comment

roberth Dec 5, 2023

Choose a reason for hiding this comment

edolstra commented Nov 1, 2023

fricklerhandwerk commented Nov 1, 2023

Ericson2314 commented Nov 1, 2023

edolstra commented Nov 3, 2023

infinisil commented Nov 3, 2023

fricklerhandwerk commented Nov 3, 2023

roberth Nov 6, 2023

Choose a reason for hiding this comment

nixos-discourse commented Nov 9, 2023

fricklerhandwerk commented Dec 4, 2023

infinisil left a comment

Choose a reason for hiding this comment

roberth left a comment

Choose a reason for hiding this comment

roberth commented Dec 10, 2023

DavHau commented Dec 27, 2023

DavHau commented Dec 27, 2023 • edited Loading

document `fetchTree` #9258

document `fetchTree` #9258

fricklerhandwerk commented Oct 30, 2023 •

edited

Loading

tomberek Oct 31, 2023 •

edited

Loading

DavHau commented Dec 27, 2023 •

edited

Loading