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

Enable glob syntax (/...) in ignored packages #709

Closed
wants to merge 2 commits into from
Closed

Enable glob syntax (/...) in ignored packages #709

wants to merge 2 commits into from

Conversation

darkowlzz
Copy link
Collaborator

Change gps.RootManifest.IgnoredPackages() to receive gps.SolveParameters.
Use the SolveParameters's RootDir and RootPackageTree to ignore
packages from root of the project.

Fixes #691

@darkowlzz darkowlzz force-pushed the 691-ignored-glob-syntax branch 2 times, most recently from cf5739a to b17acda Compare June 2, 2017 22:18
@darkowlzz
Copy link
Collaborator Author

This PR enables using golang glob syntax (/...) for ignoring local packages, refer #691 for an example scenario.

Local package path in ignored is expected to be relative to the project root.

ignored = ["./sample", "./examples/..."]

External package path would remain the same, like github.com/foo/bar.

The locally ignored packages are not used as inputs for generating inputs-digest at the moment. Should we? I see gps/hash.go ignores any package that's from project root. I think we should consider locally ignored packages too, because their imports, ignored or not ignored change the lock file. Please correct me if I'm missing something.

@darkowlzz darkowlzz changed the title [WIP] Enable glob syntax (/...) in ignored packages Enable glob syntax (/...) in ignored packages Jun 3, 2017
darkowlzz added 2 commits June 5, 2017 02:26
@darkowlzz
Enable glob syntax (/...) in ignored packages
def19da 
Change gps.RootManifest.IgnoredPackages() to receive gps.SolveParameters.
Use the SolveParameters's RootDir and RootPackageTree to ignore
packages from root of the project.
@darkowlzz
Improve ignore glob handling and add tests
024505e 
Improves handling of ignored local packages.

Adds tests for ignoring local packages.
- ensure/empty/case4 for ignoring local packages with glob
- ensure/empty/case5 for ignoring a single local package, but not
subpackages
@darkowlzz darkowlzz force-pushed the 691-ignored-glob-syntax branch from b17acda to 024505e Compare June 4, 2017 20:57
sdboyer
sdboyer suggested changes Jun 15, 2017
View reviewed changes
Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

i don't think there's going to be a way around having to do this directly in the solver itself 😢

internal/gps/manifest.go
@@ -103,7 +103,7 @@ func (m simpleRootManifest) TestDependencyConstraints() ProjectConstraints {
func (m simpleRootManifest) Overrides() ProjectConstraints {
return m.ovr
}
func (m simpleRootManifest) IgnoredPackages() map[string]bool {
func (m simpleRootManifest) IgnoredPackages(SolveParameters) map[string]bool {
Copy link
Member

Choose a reason for hiding this comment

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

Whatever route we take to accomplishing this goal, this interface needs to remain unchanged. In particular, we don't want to muddy the waters around SolveParameters by reusing it in this way.

@darkowlzz
Copy link
Collaborator Author

I'll try some more and see if there's a way to do this.

@sdboyer regarding the syntax for ignoring local packages, do we go for relative path

ignored = ["./sample", "./examples/..."]

or, full import path ?

ignored = ["github.com/foo/bar/sample", "github.com/foo/bar/examples/..."]

I think right now it works with full import path.

@sdboyer
Copy link
Member

sdboyer commented Jul 26, 2017

sorry for long delay - definitely gonna be the full import path.

also, i think that ... may not be the right syntax here, as it would strongly suggest that we're replicating the meaning of ... as its accepted by the go tooling. maybe we do, maybe we don't, but if we use that syntax, we have to make sure the alignment is intuitive.

@carolynvs carolynvs mentioned this pull request Jul 31, 2017
Merged
@sdboyer sdboyer mentioned this pull request Aug 7, 2017
Closed
@ibrasho ibrasho mentioned this pull request Aug 23, 2017
Closed
@sdboyer
Copy link
Member

sdboyer commented Aug 27, 2017

uggghhh i forgot this is here. making a TODO to pick it back up.

@darkowlzz
Copy link
Collaborator Author

Closing this in favor of #1156

@darkowlzz darkowlzz closed this Sep 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sdboyer sdboyer sdboyer requested changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@darkowlzz @sdboyer @googlebot