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

Basic lockfile support #206

Merged
merged 12 commits into from
Sep 25, 2018
Merged

Basic lockfile support #206

merged 12 commits into from
Sep 25, 2018

Conversation

travisperson
Copy link
Collaborator

This is basic support for a lock-file concept discussed between @Stebalien and @whyrusleeping this week.

See also: whyrusleeping/gx-go#49

Language string
Deps map[string]LockDep
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Per our discussion:

type Lock struct {
    // Empty for the top level package.
    // In the future, we could use IPNS!
    Ref string
    Language string

    // maps language namespace -> name in language -> dependency
    Deps map[string]map[string]Lock
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

(or we could keep it non-recursive, that was just a thought)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the "double map": The initial proposal was "dep name in language" -> definition. For example:

{
    "github.com/ipfs/go-ipfs": {
        "ref": "/ipfs/....",
        "language": "go"
    }
}

Unfortunately, this falls apart when we need multiple namespaces. For example, with CGO, we might want (not well thought out):

{
    "github.com/ipfs/go-ipfs": {
        "ref": "/ipfs/....",
        "language": "go"
    },
    "libs/libsnark.a": {
        "ref": "/ipfs/....",
        "language": "rust"
    }
}
package thing

// #cgo LDFLAGS: -L${SRCDIR}/libs -llibsnark
import "C"

The problem here is that there's the "go package" namespace and the "cgo" namespace (really, just the filesystem) and we need some way to disambiguate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The double map would be:

{
    "go": {
        "github.com/ipfs/go-ipfs": {
            "ref": "/ipfs/....",
            "language": "go"
        }
    },
    "cgo": { // files?
        "libs/libsnark.a": {
            "ref": "/ipfs/....",
            "language": "rust"
        }
    }
}

@whyrusleeping
Copy link
Owner

Added a version field here. Let's bikeshed a wee bit more, and then just pick something that works well enough for now.

@whyrusleeping
Copy link
Owner

Alright, now lock-install fetches all the dependencies to their gx paths inside the cache directory (which is currently just $PWD/vendor) and then symlinks everything into place. The files written into the cache directory are set read only. This works to build both ipfs and filecoin now!

gxutil/pm.go Outdated
return pm.installLock(lck, location, make(map[string]bool))
}

func (pm *PM) installLock(lck *LockFile, location string, complete map[string]bool) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to have pretty much the same functionality as fetchDependencies with the difference that it traverses a LockFile instead of a Package, could we leverage it? (maybe having a more abstract function that receives a closure that knows how to extract the dependency hashes and install paths?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, absolutely.

main.go Outdated
}

var lck gx.LockFile
err := gx.LoadLockFile(&lck, path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming that this naming model was inherited from LoadPackageFile but could we avoid naming the main -> gxutil functions the same? (Or add some comments that clearly distinguish them both?) This can be a bit confusing for a new user.


const LockVersion = 1

type LockDep struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we document what's the difference between a LockDep and a Dependency?

I mean, why are we using this new structure instead of extending Dependency or including it in this structure hierarchy? I'm not saying it should, but reading this automatically makes me thing of what we now call a dependency (inside the package.json), and now I'll be seeing a different file that also talks about dependencies but with a different structure which can be confusing.

I understand that conceptually both are referring to a package (that is a dep of another package) but with a different representation in a different context so it would be nice to make that link stronger.

@travisperson
Copy link
Collaborator Author

I updated this branch to use the recursive structure and implemented a rather crude worker pattern that can probably be updated a bit. It was the only way I could see doing it that wouldn't run into lock contention (though that might be able to be solved with extending the workers). Both the package install and lock install might be able to share some code if we supply a postFetch callback method as pointed out by @schomatis. I may look into this a bit further.

However, a few things immediately become apparent as I started to work on this code a bit.

  1. Making gx packages read-only breaks some hooks as they write a file to keep track
    I'm noting this because it was breaking some tests. However, this might not actually be an issue, and instead is just an artifact of not cleaning up tests. I believe only the post-install hook does this, which may not be an issue.

  2. Recursive / transient deps
    While we can keep track of the entire dependency tree in the lock-file, it currently has to be flattened entirely if we want to keep gx deps entirely read-only, as we wouldn't be able to use the dependencies "vendor" directory to store it's own / conflicting dependencies.

    In the go case I don't think this is an issue. However, if we want to support more languages we'd need to handle this case. This might be as simple as allowing the "vendor" directory to be writable. Doing this though breaks some ideas down the road, such as building directly from /ipfs / /ipns.

Regardless I think this definitely shows that the language extensions of gx need to be more in the loop with regard to how the lock file is installed.

@schomatis
Copy link
Collaborator

  1. Making gx packages read-only breaks some hooks as they write a file to keep track
    However, this might not actually be an issue, and instead is just an artifact of not cleaning up tests.

Yes, gx drops a .gx/post-install file to mark the package as installed: #208.

@whyrusleeping
Copy link
Owner

@Stebalien Are we happy with the lockfile format? I'd like to get things moving forward here.

If the lockfile format looks good, we can change the particulars of install (and lockfile generation) later.

@Stebalien
Copy link
Collaborator

I am. Let's merge this.

@whyrusleeping whyrusleeping merged commit cccb788 into master Sep 25, 2018
@whyrusleeping whyrusleeping deleted the feat/gx-lock branch September 25, 2018 17:10
@whyrusleeping
Copy link
Owner

Thanks a bunch @travisperson ! This is exciting :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants