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.

Sign up for GitHub

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

Jump to bottom

Migrate ipfs v0.7.0 to v0.8.0 #112

Merged
merged 14 commits into from
Dec 8, 2020
Merged

Migrate ipfs v0.7.0 to v0.8.0 #112

merged 14 commits into from
Dec 8, 2020

Conversation

gammazero
Copy link
Contributor

  • Convert from mdag pinning to datastore pinning

@gammazero gammazero added the status/blocked Unable to be worked further until needs are met label Nov 4, 2020
@gammazero gammazero requested a review from aschmahmann November 4, 2020 07:44
@gammazero
Migrate ipfs v0.7.0 to v0.8.0
197e400 
- Convert from mdag pinning to datastore pinning
- Build ipfs command from specific commit (for now)
- Require go1.14 in CI needed for above
- Do not build using vendor dir
@gammazero gammazero force-pushed the feat/10-11-datastore-pins branch from 91d1287 to 197e400 Compare November 6, 2020 23:33
aschmahmann
aschmahmann reviewed Nov 12, 2020
View reviewed changes
go.mod Outdated
Comment on lines 5 to 17
require (
github.com/ipfs/go-blockservice v0.1.3
github.com/ipfs/go-datastore v0.4.5
github.com/ipfs/go-ds-flatfs v0.4.5
github.com/ipfs/go-ds-leveldb v0.4.2
github.com/ipfs/go-filestore v1.0.0
github.com/ipfs/go-ipfs-blockstore v1.0.1
github.com/ipfs/go-ipfs-exchange-offline v0.0.1
github.com/ipfs/go-ipfs-pinner v0.0.5-0.20201029233447-cad837873ee6
github.com/ipfs/go-ipld-format v0.2.0
github.com/ipfs/go-merkledag v0.3.2
golang.org/x/net v0.0.0-20200114155413-6afb5195e5aa
)
Copy link
Contributor

Choose a reason for hiding this comment

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

@Stebalien WDYT is this fine for now and then we move to implementing #98, or should we just do that now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: there might be alternatives to #98 such as vendoring the dependencies and giving them all migration-specific names so we can keep everything in one binary, but I'm not sure if they'd be any better.

Copy link
Member

Choose a reason for hiding this comment

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

We can't have unvendored deps in this repo but we could try using go mod vendoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

The go mod vendor command constructs a directory named vendor in the main module's root directory that contains copies of all packages needed to support builds and tests of packages in the main module.

Is the suggestion to make new packages + go.mod's in each migration and then to try and vendor, or to copy the approach from the migrations before we were using gx.

@gammazero
Vendor deps
61e4ddd 
This is done by turning the ipfs-10-to-11 migration into its own module and vendoring its dependencies using go mod vendor.  Since this makes ipfs-10-to-11 an independent module, and no longer a package within the fs-repo-migrations module, it changes how to build it.

The top level fs-repo-migrations module must now require the ipfs-10-to-11 module in its go.mod, and then use a replace direct so that it does not actually need to download that module from github (or find it in the vendor directory).

The sharness tests must also build it as a separate module and not as a package within the fs-repo-migrations module.
aschmahmann
aschmahmann reviewed Nov 12, 2020
View reviewed changes
ipfs-10-to-11/go.mod Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member

I've manually vendored the deps.

@gammazero
Copy link
Contributor Author

One of the problems with using go modules for vendoring the dependencies of ipfs-10-to-11, and making ipfs-10-to-11 into a module, is that building fs-repo-migrations requires the the github.com/ipfs/fs-repo-migrations/ipfs-10-to-11 module. Unfortunately, ipfs-10-to-11 requires the github.com/ipfs/fs-repo-migrations. This creates a circular dependency

I think the correct solution to this is presented in #98 which gets rid of the monolithic fs-repo-migrations binary. As a temporary solution ipfs-10-to-11 can depend on a previous version of fs-repo-migrations which had no changes to the packages imported, and did not depend on ipfs-10-t0-11. This does not seem like a sustainable way of doing things, particularly if manual vendoring is required. (huge thanks to @Stebalien for doing that in this PR)

@gammazero
Copy link
Contributor Author

Note: Needed to update go version in top-level go.mod to go 1.14 to fix "duplicate method" error when compiling go-ipfs for the 10-to-11 migration. That error is caused by using the go 1.13 spec specified in go.mod. See here for details.

@gammazero gammazero added status/blocked Unable to be worked further until needs are met and removed status/blocked Unable to be worked further until needs are met labels Nov 17, 2020
@gammazero gammazero force-pushed the feat/10-11-datastore-pins branch from e70d220 to 1024b44 Compare December 4, 2020 21:33
@aschmahmann aschmahmann removed the status/blocked Unable to be worked further until needs are met label Dec 8, 2020
@aschmahmann aschmahmann merged commit a281eca into master Dec 8, 2020
@gammazero gammazero deleted the feat/10-11-datastore-pins branch December 8, 2020 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschmahmann aschmahmann aschmahmann left review comments

@Stebalien Stebalien Stebalien left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gammazero @Stebalien @aschmahmann