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

Stop using go/build, implement our own from go/parser #99

Closed
sdboyer opened this issue Sep 15, 2016 · 3 comments
Closed

Stop using go/build, implement our own from go/parser #99

sdboyer opened this issue Sep 15, 2016 · 3 comments

Comments

@sdboyer
Copy link
Owner

sdboyer commented Sep 15, 2016

There are a few reasons why relying on build.ImportDir(), even with a carefully mangled build.Context object, doesn't do what we need. The single most important thing is that we need to take a combinatorial view of build tags, primarily os/arch. This gets potentially explode-y, but my current thinking is that we'd want a distinct logical "Package" for each combination of build tags that corresponds to a unique file set.

@freeformz
Copy link
Contributor

I am playing with gps to better understand it, so I hacked in some support for gps to godep (in parallel with it's normal work), which saves the gps vendored bits to vendor_gps. This way I can diff -ur vendor vendor_gps and figure out what's different.

My hack uses a ./Godeps/Godeps.json file as lock data for the solver.

When it tries to solve the existing deps it works for everything except x/tools/go/vcs (locked to 1f1b3322f67af76803c942fd237291538ec68262), instead getting f8f91591b720f65816369d36e577b525d0926902, which is very old.

Turning on trace I see this...

| | | ✓ select github.com/kr/fs@2788f0dbd16903de03cb8186e5c7d97b69ad387b w/1 pkgs
| | | | ? attempt golang.org/x/tools with 1 pkgs; at least 1 versions to try
| | | | | try golang.org/x/tools@1f1b3322f67af76803c942fd237291538ec68262
| | | | ✗ /Users/emuller/.godep/cache/sources/https---go.googlesource.com-tools/cmd/fiximports/testdata/src/old.com/bad/bad.go:2:43: expected 'package', found 'EOF'
| | | | | try golang.org/x/tools@master
| | | | ✗ /Users/emuller/.godep/cache/sources/https---go.googlesource.com-tools/cmd/fiximports/testdata/src/old.com/bad/bad.go:2:43: expected 'package', found 'EOF'
| | | | | try golang.org/x/[email protected]

FWIW: stuff like this is why godep stopped using build.Context for parsing stuff and I ended up having to write my own parser.

sdboyer added a commit that referenced this issue Oct 14, 2016
Prior to this, encountering malformed Go source code in any package
woudl cause the entire ListPackages operation to fail. Now, when the Go
source text scanner encounters an error, we catch it and store it
correctly in a PackageOrErr.

This may well not be exhaustive - we may need to cover other types.
Handles one aspect of #99.
@sdboyer
Copy link
Owner Author

sdboyer commented Oct 14, 2016

That commit should fix the EOF issue, containing the damage to just the problem package.

sdboyer added a commit that referenced this issue Jan 20, 2017
Because xtests (package *_test) can import the package they cohabit a
directory with, but ExternalReach() merges normal and test imports
together (and ListPackages() merges test and xtest imports into test
imports), it's possible for it to appear like there's an import cycle,
even though there isn't one in code that would actually compile
together.

The proper solution to this requires refactoring to allow multiple
packages to exist per directory. That's a major undertaking, and really
should only be attempted as part of #99. So, this is a quick fix in the
meantime.
@sdboyer
Copy link
Owner Author

sdboyer commented Feb 2, 2017

While there's still work to do here, we've accomplished the basic goal - no more reliance on build.ImportDir.

@sdboyer sdboyer closed this as completed Feb 2, 2017
krisnova pushed a commit to krisnova/dep that referenced this issue Apr 21, 2017
Prior to this, encountering malformed Go source code in any package
woudl cause the entire ListPackages operation to fail. Now, when the Go
source text scanner encounters an error, we catch it and store it
correctly in a PackageOrErr.

This may well not be exhaustive - we may need to cover other types.
Handles one aspect of sdboyer/gps#99.
krisnova pushed a commit to krisnova/dep that referenced this issue Apr 21, 2017
Because xtests (package *_test) can import the package they cohabit a
directory with, but ExternalReach() merges normal and test imports
together (and ListPackages() merges test and xtest imports into test
imports), it's possible for it to appear like there's an import cycle,
even though there isn't one in code that would actually compile
together.

The proper solution to this requires refactoring to allow multiple
packages to exist per directory. That's a major undertaking, and really
should only be attempted as part of sdboyer/gps#99. So, this is a quick fix in the
meantime.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants