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

Omit manifest entry for imported projects with no source/branch/version #1373

Closed
carolynvs opened this issue Nov 13, 2017 · 5 comments · Fixed by #1414
Closed

Omit manifest entry for imported projects with no source/branch/version #1373

carolynvs opened this issue Nov 13, 2017 · 5 comments · Fixed by #1414

Comments

@carolynvs
Copy link
Collaborator

Currently when dep can't infer a constraint during import for a project, gps.Any() is used and an entry is added to the manifest that looks like this:

[[constraint]]
  name = "gitub.com/carolyn/ishangry"

Since we are going to start warning about these types of near-empty entries soon (see #1336), we should instead omit them from the manifest during import.

@arbourd
Copy link
Contributor

arbourd commented Nov 14, 2017

Would love to give this a shot this weekend :)

@carolynvs
Copy link
Collaborator Author

@arbourd That would be great! 👍 It's holding up another PR (#1336) and I would love some help.

I believe that you will want to make the change in internal/importers/base/importer.go so that all of the importers get this for free. Probably in the ImportPackages function, but that's off the top of my head. Feel free to ask questions here or open a WIP PR.

@arbourd
Copy link
Contributor

arbourd commented Nov 19, 2017

Hey @carolynvs! I finally got around to looking at this last night and I definitely need some help. Here's my understanding of internal/importers/base/importer.go:

  1. anyConstraint is default constraint if ConstraintHint is "".
  2. anyConstraint is set if the constraint is pinned
  3. anyConstraint is set if the constraint would invalidate the locked version
  4. Because of 1, any failure to learn about the constraint from its revision, if it is part of a tag, etc, fails, it stays as anyConstraint.

In what instance would we not want to to pass an anyConstraint into the manifest's list of constraints? My guess would be 4. Should 2 and 3 still attempt solving, even if the constraint may be empty?

Passing a noneConstraint type into the list of constraints is not an option, because it has the ability to break the solver.

This commit is what I'm thinking:

  • Set the default for an empty constraint string as emptyConstraint
  • Allow a constraint that is pinned to be anyConstraint
  • continue and omit the constraint from the manifest if the constraint is still empty (before checking the lock because an empty constraint will fail testConstraint, and thus return anyConstraint)

Am I on the right track? Thanks! 😁

@carolynvs
Copy link
Collaborator Author

Sorry for the slow response. The requirement is to identify projects in the manifest that:

  1. Are the any constraint, i.e. they don't specify a branch, revision or version
  2. Do not specify a source

These constraints result in an "empty" entry in the manifest

[[constraint]]
  name = "github.com/example/A"

They actually aren't necessary for the solver; the solver will include them in its calculations anyway because of the import statement analysis. So we would like to identify these empty constraints during import and remove them from the manifest before solving. That way they won't be included in the final manifest.

@arbourd
Copy link
Contributor

arbourd commented Nov 27, 2017

They actually aren't necessary for the solver; the solver will include them in its calculations anyway because of the import statement analysis.

This is something I neglected to think about or test. Thanks for the feedback.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants