Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Add analyzer and getter for source manager #7

Closed
wants to merge 3 commits into from
Closed

Add analyzer and getter for source manager #7

wants to merge 3 commits into from

Conversation

sdboyer
Copy link
Member

@sdboyer sdboyer commented Oct 18, 2016

Two things in this:

  • analyzer, which is the simplest possible implementation of a gps.ProjectAnalyzer. That's the thing that's responsible for pulling manifest and lock data out of dependency projects.
  • A dumb-as-rocks singleton setup of a gps.SourceManager, which is going to be needed for all solving operations, and will have quite a number of uses outside of that, as well.

I took care in gps to ensure that the default gps.SourceManager (*SourceMgr, returned from gps.NewSourceManager()) is completely encapsulated - no package-level state. (There's still some work to be done related to exclusivity controls at the filesystem level, as well as signal handling, but we don't have to care about that right now). That makes it safe to take this dumb approach to setting up our sm.

Longer term, though, it'd probably be preferable to inject the sm we use, rather than relying on a global.

This is the minimum amount necessary to get a SourceManager working.
@sdboyer
Copy link
Member Author

sdboyer commented Oct 18, 2016

Also worth noting that this does codify a relationship with GOPATH - we add a new dir, $GOPATH/depcache, for local copies of source repositories.

@freeformz
Copy link

I'm not sure what the point of getSourceManager is. Either have a singleton and use it or don't. If you have a singleton any setup should be done in init() IMO.

@sdboyer
Copy link
Member Author

sdboyer commented Oct 18, 2016

Totally fair point. I had two reasons for not doing it in init():

some commands - e.g. init - may not actually need the source manager. And, because setting up a *SourceMgr entails writing out a file to disk (to maintain single-process access to the cache dirs), that's something I try to avoid unless it's strictly necessary.

Second, because *SourceMgr.Release() needs to be called in order to remove that lock file, the point at which the manager gets initialized is important - you generally want to defer sm.Release() right afterwards. Controlling responsibility over that becomes even more awkward when that's done in init().

@sdboyer
Copy link
Member Author

sdboyer commented Oct 18, 2016

Hmm...but...that Release() is still awkward to manage, as this PR is currently written. Maybe it'd be better to drop the package-level var and just have getSourceManager() try to set up a new one each time. Then its only purpose is to apply defaults, not control instance access.

Yeah, I like that better.

@sdboyer
Copy link
Member Author

sdboyer commented Oct 18, 2016

Also LOL i wasn't actually even writing to the sm package var

"github.com/sdboyer/gps"
)

func getSourceManager() (*gps.SourceMgr, error) {

Choose a reason for hiding this comment

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

If I call this twice, I assume that I get an error on the second call based on what we discussed in slack?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct. it would be a gps.CouldNotCreateLockError, which has enough info in it to differentiate from e.g. fs perm problems

Copy link
Member Author

Choose a reason for hiding this comment

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

So now, you'd have to pick one place that you create the sm, which then is also the point where you have to decide about Release()ing it.

@sdboyer sdboyer changed the title Analyzer and source manager singleton Add analyzer and getter for source manager Oct 18, 2016
@freeformz
Copy link

tbh I would just push all of this into a PR that actually uses it.

@sdboyer
Copy link
Member Author

sdboyer commented Oct 18, 2016

sure, i can do that, too

@sdboyer sdboyer mentioned this pull request Oct 18, 2016
@sdboyer
Copy link
Member Author

sdboyer commented Oct 18, 2016

Closing in favor of #8

@sdboyer sdboyer closed this Oct 18, 2016
zbintliff added a commit to zbintliff/dep that referenced this pull request Mar 3, 2017
ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017
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.

2 participants