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

Clarify ipld/car relatioship with ipld/go-car #244

Closed
wants to merge 3 commits into from
Closed

Conversation

BigLep
Copy link
Contributor

@BigLep BigLep commented Mar 30, 2023

See conversation in #218 (comment)

@BigLep BigLep self-assigned this Mar 30, 2023
@BigLep BigLep requested a review from a team as a code owner March 30, 2023 20:40
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #244 (1fcb4ff) into main (085ed9d) will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
- Coverage   48.23%   48.13%   -0.11%     
==========================================
  Files         270      270              
  Lines       33064    33064              
==========================================
- Hits        15950    15915      -35     
- Misses      15455    15482      +27     
- Partials     1659     1667       +8     

see 10 files with indirect coverage changes

@willscott
Copy link
Contributor

The dependency issue here seems to be on the blocks library and whether to use the boxo instance versus the older one in go-block-format.

There are only a couple calls / references in there, and the definition of what a block is will be really quite stable at this point.

go-car could remove this dependency with a bit of refactoring by defining the interface locally, so that it can be compatible with either of these implementations.

If there wasn't a dependency re-factor on ipld/go-car would boxo be philosophically okay with using that as a canonical version, or is the hope to pursue divergence per the reference to having this implementation "thinned out"?
Is there an issue or reference to what complexity in the current car library we hope can be removed?

@BigLep
Copy link
Contributor Author

BigLep commented Mar 31, 2023

The dependency issue here seems to be on the blocks library and whether to use the boxo instance versus the older one in go-block-format.

I believe tha's right.

go-car could remove this dependency with a bit of refactoring by defining the interface locally, so that it can be compatible with either of these implementations.

Cool!

If there wasn't a dependency re-factor on ipld/go-car would boxo be philosophically okay with using that as a canonical version, or is the hope to pursue divergence per the reference to having this implementation "thinned out"?

Per #218 (comment) , go car libraries isn't where we were wanting to apply maintenance or innovation energy. I know the team is comfortable building and working with car tooling if necessary (e.g., @Jorropo's different car related libraries in https://github.com/Jorropo/linux2ipfs and https://github.com/Jorropo/go-featheripfs).

I expect the team will want a break for a month or two before considering getting rid of boxo/ipld/car given the fatigue on all the renaming and path updating the last month.

I know I'm being non-committal here, not because have another secret plan, but because don't know at this point.

Is there an issue or reference to what complexity in the current car library we hope can be removed?

Good point. I don't think so: https://github.com/ipld/go-car/issues?q=is%3Aissue+jorropo+
I'll ask @Jorropo to do this now...

@willscott
Copy link
Contributor

Per #218 (comment) , go car libraries isn't where we were wanting to apply maintenance or innovation energy.

The angle I'm taking here is that there have been a number of PRs over the last year to fix bugs or otherwise improve the car-v1 codebase that is in the go-car repo (example 1 2 3). By having a separate instance in boxo, there's a duplicate maintenance burden we're incurring.

In the meantime we probably want to have some sense of the expected process: - is it on go-car to notify boxo that there's a PR there that probably should get pulled into the boxo implementation and vice versa? or should the boxo maintainers be watching go-car for such relevant PRs and vice versa?

Adjust migration tool config so that don't update users to boxo/ipld/car by default.
@willscott
Copy link
Contributor

The dependency issue here seems to be on the blocks library and whether to use the boxo instance versus the older one in go-block-format.

I believe tha's right.

I looked into potentially providing an interface, but this seems to not be the case.

The boxo fork of car is using the same external go-block-format interface as ipld/go-car 1.

As such, I don't think there's a refactor-needed / incompatibility in linking against the external ipld/go-car as it is. If that isn't true we should track down what it is we are proposing needs to be done.

@BigLep
Copy link
Contributor Author

BigLep commented Apr 6, 2023

we probably want to have some sense of the expected process: - is it on go-car to notify boxo that there's a PR there that probably should get pulled into the boxo implementation and vice versa? or should the boxo maintainers be watching go-car for such relevant PRs and vice versa?

Good point @willscott - thanks for raising.

I think Boxo in general needs a mechanism here to know if all the copied repos (including ipld/go-car) have received "important updates". I created #270 to track this work (and included an for how dependabot could be leveraged).

For ipld/go-car specifically, I don't want you and the current maintainers to have to do anything extra than you already do for notifying consumers of releases/changes. I assume that happens through the normal github release and dependabot flow, but if there is anywhere else we should be watching let us know. Certainly if there are important bug fixes or performance improvements, we're not going to say "no" to extra communication in #boxo-maintainers FIL slack or ipfs/boxo issue, but that is not expected.

Thanks!

@willscott
Copy link
Contributor

That makes sense - my angle here has been to make sure that if nothing else, the pull from the ipld instance can remain as trivial as possible - so making sure we understand what would prevent the code from being identical between the two.

soon → in the future since there aren't specific plans here
(if there were, there should be an issue describing it)
@BigLep
Copy link
Contributor Author

BigLep commented Apr 6, 2023

@willscott : concerning go-block-format I'll need @Jorropo to chime in. I believe the issue manifests due to nested types, but I'll let others speak with authority here.

@BigLep
Copy link
Contributor Author

BigLep commented Apr 13, 2023

Remaining work here @Jorropo :

  1. Give a shipit / merge on the PR itself
  2. Make it clear what ipld/go-car would need to change dependency-wise so Boxo could depend on it again. I think it would be nice if we were in a position of not needing to copy that code because of dependencies, but I can understand if we keep the copy because of different needs.
  3. ✅ Specify what you'd like ipld/go-car to ideally do (you've done this per Feedback/proposal: separate concerns more and provide a safe wrapper around the car format ipld/go-car#416 )

@BigLep
Copy link
Contributor Author

BigLep commented May 1, 2023

Per IPFS Thing 2023 conversations, we are going to be able to remove Boxo's ipld/go-car copy from Boxo. When that happens, this PR can just be closed.

@BigLep
Copy link
Contributor Author

BigLep commented Jun 13, 2023

I'm closing this PR since we'll be removing boxo/car per #218 (comment)

@BigLep BigLep closed this Jun 13, 2023
@Jorropo Jorropo deleted the biglep/clarify-car branch June 13, 2023 23:48
@Jorropo Jorropo restored the biglep/clarify-car branch June 13, 2023 23:48
@hacdias hacdias deleted the biglep/clarify-car branch June 27, 2023 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants