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

Initial IPFS support. #2589

Closed
wants to merge 22 commits into from
Closed

Initial IPFS support. #2589

wants to merge 22 commits into from

Conversation

Ambrevar
Copy link
Member

@Ambrevar Ambrevar commented Sep 23, 2022

Description

Add support for IPFS web page browsing. To test, just load ipfs://bafybeiemxf5abjwjbikoz4mc3a3dla6ual3jsgpdr4cjr3oz3evfyavhwq/wiki/Vincent_van_Gogh.html.

Partially fixes #1064.

To do:

  • Add support for adding / pinning files and directories.

    For directory support, upstream fixes are needed (my forks have them).

    Mostly done, needs testing.

    Similar to what https://github.com/ipfs/ipfs-companion does. We need to persist the list of added / pinned files.

  • Add IPNS support.

  • Parse response headers.

Discussion

Daemon management is not very robust for now, needs to be improved. Suggestions welcome.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

  • I have pulled from master before submitting this PR
  • There are no merge conflicts.
  • I've added the new dependencies as:
    • ASDF dependencies,
    • Git submodules,
      cd /path/to/nyxt/checkout
      git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser
    • and Guix dependencies.
  • My code follows the style guidelines for Common Lisp code. See:
  • I have performed a self-review of my own code.
  • My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
  • Documentation:
    • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
    • I have updated the existing documentation to match my changes.
    • I have commented my code in hard-to-understand areas.
    • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
    • I have added a migration.lisp entry for all compatibility-breaking changes.
  • Compilation and tests:
    • My changes generate no new warnings.
    • I have added tests that prove my fix is effective or that my feature works. (If possible.)
    • New and existing unit tests pass locally with my changes.

Copy link
Contributor

@aartaka aartaka left a comment

Choose a reason for hiding this comment

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

All good, except #2588 somewhy being in the diff :P

Let me test it.

@@ -131,9 +131,9 @@ issued by Control+<button1> in a new window.")
(downloads
:documentation "List of downloads. Used for rendering by the download manager.")
(startup-timestamp
(local-time:now)
(time:now)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, does this belong to this PR? #2588 is the one it's from, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because I based my work on it. Don't worry, it will disappear from this PR when #2588 is merged.

(:toggler-command-p nil))

(defmethod daemon-running-p ((mode ipfs-mode)) ; Unused? Useful for debugging though.
(= 0
Copy link
Contributor

Choose a reason for hiding this comment

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

zerop :)

(:a url)
" to "
(:a new-url))))))
(buffer-load new-url :buffer (buffer mode))
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually make it in a separate thread (run-thread) with a slight timeout ((sleep 0.5)) so that the message is visible.

@aartaka
Copy link
Contributor

aartaka commented Sep 23, 2022

It works! :D

@Ambrevar
Copy link
Member Author

Note: For now my forks of Drakma and cl-ipfs-api2 are needed. There are a bunch of fixes I'm working on with upstream:

@aadcg
Copy link
Member

aadcg commented Oct 13, 2022

What's the status here?

@jmercouris
Copy link
Member

It is ready to merge, just want to see some changes in upstream libraries.

@Ambrevar
Copy link
Member Author

Indeed. Plus please review the code, I believe no one has done it.

@Ambrevar
Copy link
Member Author

IPNS support can be added later.

Copy link
Contributor

@aartaka aartaka left a comment

Choose a reason for hiding this comment

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

But I did review it before! Anyway, here're my two cents! Mostly style and library suggestions, though.

Return immediately if already started."
(sera:synchronized ((daemon mode))
(when (and (not (daemon mode))
(sera:resolve-executable (program mode)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a premature optimization, but: the pattern of storing executable names and resolving them appears several times in our code. Could this be worth of nfiles?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! See atlas-engineer/nfiles#8

(sera:synchronized ((daemon mode))
(when (and (not (daemon mode))
(sera:resolve-executable (program mode)))
(let ((p-i (uiop:launch-program (append (list (program mode) "daemon")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use cons to avoid this (append (list ...) ...)?

It's not exactly pretty and SICL style guide discourages using cons for anything but creating conses, though.

(bt:with-timeout ((daemon-timeout mode))
(loop
(unless (uiop:process-alive-p p-i)
(error "Already started?"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a dedicated error type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, this part is ugly, but they are so many edge cases and it's not too clear how we want to behave... Need to think this through.

(progn
(bt:with-timeout ((daemon-timeout mode))
(loop
(unless (uiop:process-alive-p p-i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use loop's when and else keywords? It reduces nesting and makes things look more coherent.

Example rewrite:

(loop
  when (and (uiop:process-alive-p p-i)
            (string= "Daemon is ready" (read-line (uiop:process-info-output p-i))))
    do (return 'ready)
  else
    unless (uiop:process-alive-p p-i)
      do (error "Already started?"))

(defun ipfs-request-p (request-data)
(and (toplevel-p request-data)
(or (str:s-member '("ipfs" "ipns") (quri:uri-scheme (url request-data)))
;; TODO: It's in the response header.
Copy link
Contributor

Choose a reason for hiding this comment

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

Request headers are now inspectable on master with http-headers. Response headers, though...

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you add support for response headers in cl-webkit?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's already there, actually. And I've added it to request-data in 5aec043.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fantastic, thanks a lot!

(let ((cid (quri:uri-domain url)))
(quri:copy-uri url
:scheme "https"
:port (quri.port:scheme-default-port "https")
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be addressed in quri:copy-uri, shouldn't it? I mean, port mismatch is too easy to miss, so we need to guard against it somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

source/mode/ipfs.lisp Show resolved Hide resolved
(let ((mime (or (mimes:mime (pathname (quri:uri-path url)))
"text/html;charset=utf8")))
;; WARNING: Need byte-vector here because strings CL encoding may confuse the browser.
(values (flex:string-to-octets (ipfs:cat (nyxt::schemeless-url url))) ; TODO: Export?
Copy link
Contributor

Choose a reason for hiding this comment

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

Export, like lots of other URL utilities! Maybe even contribute it to QURI as some quri/utilities package? ( ͡° ͜ʖ ͡°)

Copy link
Member Author

Choose a reason for hiding this comment

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

(defmethod add ((mode ipfs-mode) path) ; TODO: Report error when daemon is not running.
"Add PATH to IPFS and the MFS so that it's listed by 'ipfs files ls'.
See the help message of 'ipfs files'."
(when (daemon-running-p mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move this chacking into some accessor? Seems to be suitable for a data-model-based restriction...

Copy link
Member Author

Choose a reason for hiding this comment

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

Which accessor though?

Copy link
Member Author

Choose a reason for hiding this comment

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

Friendly ping :)

Copy link
Contributor

Choose a reason for hiding this comment

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

daemon maybe? We cannot use it until it's started anyway...

(progn
(mapc (compose #'ipfs:files-rm #'ipfs-path) files)
(echo "Deleted IPFS files: ~a" (mapcar #'name files)))
(echo "Could not start IPFS daemon required to delete files."))))
Copy link
Contributor

Choose a reason for hiding this comment

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

This thing appears more than once too. Maybe abstract it to an accessor too?

@aadcg
Copy link
Member

aadcg commented Jul 5, 2023

What's the status here? CC @jmercouris.

@Ambrevar
Copy link
Member Author

Ambrevar commented Jul 6, 2023

We should probably move this to a separate WIP-extension repository.

@aadcg aadcg marked this pull request as draft September 24, 2023 13:44
@jmercouris
Copy link
Member

Please reopen when ready.

@jmercouris jmercouris closed this Oct 28, 2023
@aadcg
Copy link
Member

aadcg commented Oct 30, 2023

We should probably move this to a separate WIP-extension repository.

You should have a local copy of the branch so please backup the work. Deleting the branch on the remote.

@aadcg aadcg deleted the ipfs branch October 30, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Support Hypercore/IPFS
4 participants