Skip to content
This repository has been archived by the owner on Feb 26, 2019. It is now read-only.

list packages rather than repos #35

Merged
merged 1 commit into from
Nov 25, 2013
Merged

list packages rather than repos #35

merged 1 commit into from
Nov 25, 2013

Conversation

kr
Copy link
Member

@kr kr commented Nov 22, 2013

This small edit is a significant conceptual change.
Instead of listing one entry per dependency repo,
we list one entry per disjoint import path tree.

This means we can list the same repo more than once
in the manifest, but copy less code into the local
workspace. This happens when a repo has several
packages, not all of which are dependencies.

For example, if the project depends on packages A
and B in repo R, we now list both R/A and R/B, even
though they're in the same repo. However, if the
project depends on two packages P and P/Q, we still
list only P.

@kr
Copy link
Member Author

kr commented Nov 22, 2013

Hey @davecheney and @mreiferson, could I ask for a
quick review of this? Thoughts?

Basically this gives a more tightly focused set of dependency
packages at the cost of more entries in the manifest.

Do you like this change? Or prefer the current way?

@kr
Copy link
Member Author

kr commented Nov 22, 2013

As an example, before this change, importing package
github.com/crowdmob/goamz/sqs would result in one json entry:

        {
            "ImportPath": "github.com/crowdmob/goamz",
            "Rev": "9bee4d46f8a986ae73c9c05e7359e940f9ffa80c"
        },

And that entire repo would be copied, including all its packages:
aws, cloudwatch, dynamodb, ec2, elb, exp, iam, s3, sqs, and testutil.

With this patch, you get two entries:

        {
            "ImportPath": "github.com/crowdmob/goamz/aws",
            "Rev": "9bee4d46f8a986ae73c9c05e7359e940f9ffa80c"
        },
        {
            "ImportPath": "github.com/crowdmob/goamz/sqs",
            "Rev": "9bee4d46f8a986ae73c9c05e7359e940f9ffa80c"
        },

and only those two directories, aws and sqs, are copied.

@mreiferson
Copy link
Contributor

I like the higher granularity and more explicit references to what you're actually importing and depending on. I don't mind the "overhead" in the manifest at all.

Do you think theres a possibility that the duplication could be construed as implying that those two dependent packages could be pinned to different hashes? Should this be an explicit error condition if they diverge?

Even more radical, should the manifest (for each dependent repo) record the root, pinned hash, and an array of dependent import paths instead? Your example above becomes:

        {
            "RepoRoot": "github.com/crowdmob/goamz",
            "Rev": "9bee4d46f8a986ae73c9c05e7359e940f9ffa80c",
            "ImportPaths": [
                "github.com/crowdmob/goamz/aws",
                "github.com/crowdmob/goamz/sqs"
            ]
        }

Does this make it more obvious that there is a single repo that we depend on but we only import the specified subset of packages? I'm not sure I like it but it's food for thought.

Relatedly, I'm curious to hear your thoughts on "versioning" of godep itself. Its been fantastic that as it evolved it has maintained backwards compatibility and I think the change as you've implemented here continues that. Still, it might be important to begin to lay the groundwork so something can be embedded in the manifest identifying compatibility, making it a little easier to manage the possibility of needing to make breaking changes.

I don't want to derail this issue with the versioning conversation though, so perhaps we should open a separate issue for that discussion, I'm sure there are lots of opinions :)

@kr
Copy link
Member Author

kr commented Nov 22, 2013

Interesting point about the commit ids getting out of sync.
I hadn't considered that explicitly. I think it's out of scope
for godep. After all, there are countless other ways things
could get out of sync, if people are editing them by hand.
The code could change, the commit id could simply be
wrong, etc. As long as the json contents are generated by
'godep save', I don't see how they could get out of sync.
I'm inclined not to worry much about it.

The possible implication that they might correctly diverge
is a little more troubling. Not sure what to do to combat that
false impression.

I did think about putting a list of import paths inside the
entry for a dependency, but went with this approach because
it's a smaller change (and requires no changes to logic that
consumes the list), and out of a vague desire to keep the
structure flatter, with fewer optional or alternative fields,
and not overdo the DRY impulse.

I want it to be at least within the realm of possibility to
consume Godeps.json with things like jq and shell
scripts. So the flatter and more regular the better.

@mreiferson
Copy link
Contributor

Agreed, 👍

This small edit is a significant conceptual change.
Instead of listing one entry per dependency repo,
we list one entry per disjoint import path tree.

This means we can list the same repo more than once
in the manifest, but copy less code into the local
workspace. This happens when a repo has several
packages, not all of which are dependencies.

For example, if the project depends on packages A
and B in repo R, we now list both R/A and R/B, even
though they're in the same repo. However, if the
project depends on two packages P and P/Q, we still
list only P.
@kr kr merged commit 3ef7304 into master Nov 25, 2013
@kr kr deleted the pp branch November 25, 2013 22:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants