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

[RFC] Explicitly define in glide.lock if base package is a dependency. #314

Closed
wants to merge 1 commit into from

Conversation

sgotti
Copy link

@sgotti sgotti commented Mar 7, 2016

Working on https://github.com/sgotti/glide-vc I noticed that, using the glide.lock file, it's not possible to determine if a base package with some subpackages is itself a project dependency.

For example, given:

- name: github.com/sgotti/project01    
  version: b16f2e739216b746bda87fb5b677fa672f09afb7
  subpackages:                                     
  - subpackage01

I'm not able to determine if only github.com/sgotti/project01/subpackages01 or also github.com/sgotti/project01 are needed packages.

One of the idea that came to my mind was to explicitly add "." to the subpackages if the base package is a dependency. This is what the actual PR does to trigger an initial discussion.

This of course will make the lock file slightly more verbose since every package will have a subpackage section. An improvement to make it less verbose will be to add "." only if there're other subpackages (so if only the base package is needed, no subpackage with just "." will be added).

Actually the lock file doesn't clearly define if a base package is a
project dependency or not.

This patch makes it explicit adding "." as a subpackage if the base
package is a project dependency.
@technosophos
Copy link
Member

We've talked about this in the past. And maybe it's time to talk about it again.

To keep you from getting bogged down while we discuss, would the output of glide list help you solve the problem without a change to the glide.yaml file?

@sgotti
Copy link
Author

sgotti commented Mar 23, 2016

@technosophos sorry for being late but due to a too much aggressive gmail filter I lost some github notifications.

To keep you from getting bogged down while we discuss, would the output of glide list help you solve the problem without a change to the glide.yaml file?

Yeah, I was thinking, as an alternate solution to reuse the glide list code for doing this.

@mattfarina
Copy link
Member

@sgotti since this is machine to machine communication, would it be best to get the output of glide list as json?

@sgotti
Copy link
Author

sgotti commented Mar 24, 2016

@mattfarina I wanted to avoid parsing the current glide list output. But if glide list will provide a json output (or have I lost something and it's already available?) I think it can be used.

@sgotti
Copy link
Author

sgotti commented Apr 5, 2016

@mattfarina @technosophos For glide vc there's actually the need of two kind of information:

Now they are both provided by glide.lock but, other than what this PR tries to fix I noticed that the glide.lock can also contains unneeded subpackages if they are defined in some dependency's godep.json file. I think this is probably wanted so the solution will be to not rely on glide.lock but get the above two information somewhere.

Actually glide list just provides the first information (but I'm waiting for a json format). I can get the latter using glide's util.GetRootFromPackage but since I'm trying to avoid calling code (or I'd also just call resolver.ResolveLocal(true) to get the first information) I'd like to avoid this if possible. What about providing it in glide list json output? Other ideas?

@sdboyer
Copy link
Member

sdboyer commented Dec 8, 2016

This is gonna be happening anyway with the gps transition (a "." subpackage will be listed in the lock, when appropriate), so closing this out.

@sdboyer sdboyer closed this Dec 8, 2016
This pull request was closed.
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