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

docs/code-flow : Add code flow documentation for add cmd. #5864

Merged
merged 6 commits into from
Jan 21, 2019

Conversation

nitishm
Copy link

@nitishm nitishm commented Dec 20, 2018

License: MIT
Signed-off-by: Nitish Malhotra nitish.malhotra@gmail.com

@schomatis
Copy link
Contributor

@nitishm hey, sorry, I totally forgot about this, I've been too focused on the MFS front when I should be prioritizing documents like this one that can have a much greater impact on our community.

@schomatis schomatis mentioned this pull request Dec 30, 2018
Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

Very nice!

Let's add code references to a fixed go-ipfs version (and document it as such), e.g.,

https://github.com/ipfs/go-ipfs/tree/v0.4.18

We can use a concrete command example (this would help the reproducibility of this doc), e.g.,

# Convert a file to the IPFS format.
echo "Hello World!" > new-file
ipfs add new-file
# added QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG new-file

# Add a file to the MFS.
NEW_FILE_HASH=$(ipfs add new-file -Q)
ipfs files cp /ipfs/$NEW_FILE_HASH /new-file
# Now we don't need the file hash anymore (which will even change in the
# future if we want to modify it), we can reference it through its MFS
# path: `/new-file`.

# Get information from the file in MFS.
ipfs files stat /new-file
# QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG
# Size: 13
# CumulativeSize: 21
# ChildBlocks: 0
# Type: file
# TODO: Explain some of this fields.

# Retrieve the contents.
ipfs files read /new-file
# Hello World!

(ideally this example should be the same as the one in the Getting Started guide)

A next step that would be nice, taking advantage of your experience with Delve, could be to add an execution trace, showing the concrete values in certain function calls (we should maybe later create a different section about debugging go-ipfs), e.g.,

dlv version
Delve Debugger
Version: 1.0.0

dlv debug github.com/ipfs/go-ipfs/cmd/ipfs --build-flags "-tags noplugin" -- add new-file

# Inside `AddAllAndPin`
b coreunix/add.go:420

(dlv) c
> github.com/ipfs/go-ipfs/core/coreunix.(*Adder).AddAllAndPin() ./core/coreunix/add.go:420 (hits goroutine(30):1 total:1) (PC: 0x111fcd3)
   415:			// Iterate over each top-level file and add individually. Otherwise the
   416:			// single files.File f is treated as a directory, affecting hidden file
   417:			// semantics.
   418:			for {
   419:				f, err := file.NextFile()
=> 420:				if err == io.EOF {
   421:					// Finished the list of files.
   422:					break
   423:				} else if err != nil {
   424:					return nil, err
   425:				}
(dlv) p f
gx/ipfs/QmZMWMvWMVKCbHetJ4RgndbuEF1io2UpUxwQwtNjtYPzSC/go-ipfs-files.File(*gx/ipfs/QmZMWMvWMVKCbHetJ4RgndbuEF1io2UpUxwQwtNjtYPzSC/go-ipfs-files.ReaderFile) *{
	filename: "new-file",
	fullpath: "new-file",
	abspath: "/home/user/go/src/github.com/ipfs/go-ipfs/new-file",
	reader: io.ReadCloser(*os.File) *{
		file: *(*os.file)(0xc0000af800),},
	stat: os.FileInfo(*os.fileStat) *{
		name: "new-file",
		size: 13,
		mode: 436,
		modTime: (*time.Time)(0xc00020eab0),
		sys: (*syscall.Stat_t)(0xc00020eac8),},}


b Directory.AddChild
c
(dlv) stack
0  0x0000000000e37113 in gx/ipfs/QmUwXQs8aZ472DmXZ8uJNf7HJNKoMJQVa7RaCz7ujZ3ua9/go-mfs.(*Directory).AddChild
1  0x0000000000e3af4c in gx/ipfs/QmUwXQs8aZ472DmXZ8uJNf7HJNKoMJQVa7RaCz7ujZ3ua9/go-mfs.PutNode
2  0x000000000111f9dc in github.com/ipfs/go-ipfs/core/coreunix.(*Adder).addNode
3  0x0000000001120b48 in github.com/ipfs/go-ipfs/core/coreunix.(*Adder).addFile
4  0x000000000111ff39 in github.com/ipfs/go-ipfs/core/coreunix.(*Adder).AddAllAndPin
5  0x0000000001137a5e in github.com/ipfs/go-ipfs/core/coreapi.(*UnixfsAPI).Add

But check if this makes sense to you first, many of our functions are not very friendly and peeking inside may not reveal much useful information. Ideally, what I think would be really useful for a beginner would be (1) descriptive logs that would tell you what's going on inside without debugging and (2) external tools that would help shed some light on our internal structures, something like ipfs cid but for other components, or an extended version of ipfs object diff.

docs/add-code-flow.md Outdated Show resolved Hide resolved
docs/add-code-flow.md Outdated Show resolved Hide resolved
docs/add-code-flow.md Outdated Show resolved Hide resolved
@Stebalien Stebalien added the need/author-input Needs input from the original author label Jan 4, 2019
@nitishm
Copy link
Author

nitishm commented Jan 6, 2019

I need to get to this soon.

Will try to wrap this up by the end of next week ! (sans the delve part, which might need me to do some investigation)

@schomatis
Copy link
Contributor

Sounds good, let me know if you need any help.

@nitishm
Copy link
Author

nitishm commented Jan 7, 2019

@schomatis I cleaned up the doc and formatted it in a better way.

Cant figure out what version of go-mfs to reference (kept it as master for now. Will change after your comment) ?

@schomatis
Copy link
Contributor

Cant figure out what version of go-mfs to reference

That would be the version checked by the go-ipfs version we're documenting, in this case it would be

https://github.com/ipfs/go-ipfs/blob/aefc746f34e5ffdee5fba1915c6603b65a0ebf81/package.json#L533-L535

This is the Gx version that may not coincide with the actual go-mfs GitHub version (at some point publishing in Gx meant also tagging a release in the repo but I'm not sure now).

So, that version is not listed in the releases,

https://github.com/ipfs/go-mfs/releases

so basically what I do (there may be other ways, I hope) is to trace back the .gx/lastpubver which records the Gx version every time a gx publish is done, I go to the history,

https://github.com/ipfs/go-mfs/commits/master/.gx/lastpubver

and find the commit that corresponds to that Gx version published,

ipfs/go-mfs@8b5e8dd#diff-d95150d9db6fb31091770e6d6105b08d

where you can see the hash QmUwXQs8 that matches the one in package.json (the hash is always the ultimate source of truth in IPFS, and hence in Gx),

ipfs/go-mfs@8b5e8dd#diff-d95150d9db6fb31091770e6d6105b08dR1

Yes, this workflow definitely needs to be improved, I'm not sure why we stop tagging the Gx publishes in GitHub.

@schomatis schomatis added status/in-progress In progress and removed need/author-input Needs input from the original author labels Jan 7, 2019
@schomatis schomatis added this to the Files API Documentation milestone Jan 7, 2019
@Stebalien
Copy link
Member

Yes, this workflow definitely needs to be improved, I'm not sure why we stop tagging the Gx publishes in GitHub.

whyrusleeping/gx#157

(all existing tags are created manually or with https://github.com/whyrusleeping/gx/blob/master/contrib/gx-retrotag.sh)

@schomatis
Copy link
Contributor

Oh, too bad, I'll update the tags then.

nmalhotra added 3 commits January 10, 2019 22:19
License: MIT
Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
- Cleaned up some of the language.
- Added versioned links to the code.
- Provided examples

License: MIT
Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
- go-mfs linked to v0.1.18
- go-unifx linked to v1.1.16

However,

- go-merkledag has been linked to v1.1.13 which is not the right
version. v1.1.15 has not been released and cannot be referenced

License: MIT
Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
- Moved AddAllAndPin as a subsection of UnixfsAPI.Add()

License: MIT
Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
@nitishm
Copy link
Author

nitishm commented Jan 13, 2019

@schomatis Is there anything else you would like me to add to the docs ? The delve piece is still pending but I am not sure how do get enough information in that way. I think keeping it simple with the current format might be sufficient for now.

LMK what you think ?

Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

This is really nice, left some minor notes. I'd recommend to add:

  1. A link to this document in https://github.com/ipfs/go-ipfs/#development to give more visibility to it.

  2. A link here to https://github.com/ipfs/docs/issues/133 as a background read to get some idea of what terms like MFS and UnixFS mean (that is still a highly WIP document but it's the best we've got for the moment).

I'm still unsure what's the best format to present this kind of information but I'm in favor of getting this merged ASAP and iterate on it after we've got some feedback from the community.

docs/add-code-flow.md Outdated Show resolved Hide resolved
docs/add-code-flow.md Outdated Show resolved Hide resolved
docs/add-code-flow.md Outdated Show resolved Hide resolved
docs/add-code-flow.md Outdated Show resolved Hide resolved
docs/add-code-flow.md Outdated Show resolved Hide resolved
docs/add-code-flow.md Outdated Show resolved Hide resolved
docs/add-code-flow.md Outdated Show resolved Hide resolved
docs/add-code-flow.md Outdated Show resolved Hide resolved
- Cleaned up the document.
- Added additional references to documents.

License: MIT
Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

Few nits and ready to go.


- **[UnixFS] [`(BasicDirectory).AddChild(ctx, name, ipld.Node)`](https://github.com/ipfs/go-unixfs/blob/v1.1.16/io/directory.go#L137)** - *Add child to `BasicDirectory`*

> IMPORTANT: It should be noted that the `BasicDirectory` struct of the `UnixFS` package, encasulates a node object of type `ProtoNode`, which is a different format from the `ipld.Node` we have been working on throughout this document.
> IMPORTANT: It should be noted that the `BasicDirectory` object uses the `ProtoNode` type object which is an implementation of the `ipld.Node` interface, seen and used throughout this document. Ideally the `ipld.Node` should always be used, unless we need access tp specific functions from `ProtoNode` (like `Copy()`) that are not available in the interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo, tp -> to.


# Also see
1. https://github.com/ipfs/go-ipfs/#development
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I probably wasn't very clear, I meant for you to add a link in the https://github.com/ipfs/go-ipfs/#development section pointing here, so this document wouldn't get lost in the docs directory.

Copy link
Author

Choose a reason for hiding this comment

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

Oh you meant only in the issue conversation space and not the actual doc?! Oops!

Copy link
Contributor

Choose a reason for hiding this comment

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

In the actual entry-point README doc, but instead of pointing from here to there, point there to here.

Copy link
Author

Choose a reason for hiding this comment

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

Oh dang. I completely misunderstood. I didn't realize the link was the the top README.md doc. Alright I will go ahead and add it to the README.md

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you're right, sorry, I didn't realize the link was copied without the README.md path 😬

# Also see
1. https://github.com/ipfs/go-ipfs/#development
2. [Concept document about files](https://github.com/ipfs/docs/issues/133)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably move this to the introduction (before actually starting to talk about MFS/UnixFS).

- Fixed typo.
- Moved concept document link to top of page.
- Added link to this doc to README.md

License: MIT
Signed-off-by: Nitish Malhotra <nitish.malhotra@gmail.com>
Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for committing to this @nitishm, I know that documentation isn't the most exciting thing but I think that PRs like this can make a difference just as important as any other fix or feature 👍

@schomatis schomatis added topic/docs-ipfs Topic docs-ipfs RFM and removed status/in-progress In progress labels Jan 16, 2019
@schomatis
Copy link
Contributor

/cc @Stebalien

@Stebalien Stebalien merged commit 74bf836 into ipfs:master Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFM topic/docs-ipfs Topic docs-ipfs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants