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

[WIP] Ignore NoGoCode errors from required packages #1545

Closed

Conversation

carolynvs
Copy link
Collaborator

@carolynvs carolynvs commented Jan 18, 2018

  • Ignore NoGoCode errors from required packages.
  • Change hashing structure to consider required and imported packages separately.
  • Any other changes needed due to bumping the solver version?
  • New unit tests
    • nogocode only allowed for required's
  • Changelog

What does this do / why do we need it?

This allows someone to vendor a project that doesn't have Go code by adding an entry to the required list in Gopkg.toml.

What should your reviewer look out for in this PR?

As I worked on this I realized that the change I made to support transitive constraints from #1489 also allowed me to keep track projects that were introduced from a required, vs. an import. This plumbing enables us to pass information through the solve process and use them during checks. That is the first commit in this PR. It's the same commit from that pull request.

It's either a really great idea or something that will throw @sdboyer into a violent rage. 馃榾

Do you need help or clarification on anything?

I put bmi on atomWithPackages because it already had a bmi method to recreate the original bmi. Not sure if it's safe to live there considering the comments on the original bmi function about avoiding copys of the package list. So it may need to shift elsewhere or these extra values should be attached directly to the atom struct.

Which issue(s) does this PR fix?

Fixes #1306

carolynvs and others added 3 commits January 17, 2018 17:45
This is a prototype for keeping track of the path through the selection
process to a project. It is used to help dep ignore "stale" transitive
constraints: constraints that when created applied to a descendent but
should no longer apply now that that the project has moved to another
location in the dependency graph.

Questions:
* I put bmi on atomWithPackages because it already had a bmi method to
recreate the original bmi. Not sure if it's safe to live there
considering the comments on the original bmi function about avoiding
copys of the package list. So it may need to shift elsewhere or path
should be be split out from the bmi struct so that it can be attached
directly to an atom.
* Is a non-bimodal solve ever a possibility in dep (outside of the unit
tests?). I think we could drop "dep.isTransitive" in favor of using
bmi.path exclusively but the unit tests have a non-bimodal set of tests
that cause this to fail.
@carolynvs
Copy link
Collaborator Author

@sdboyer This has a WIP on it because it's not ready to merge but when you have time I'd appreciate a sanity check on my solver changes. I don't want to put more work on this until I know that my structural changes are acceptable.

@mcandre
Copy link

mcandre commented Feb 21, 2018

Could this PR be prioritized? Like, Go's xtools at golang.org/x/tools can't be resolved with dep until this work is completed.

@sdboyer
Copy link
Member

sdboyer commented Feb 21, 2018

Could this PR be prioritized?

This is open source - it's spelled "what can i do to help?" 馃槣

Like, Go's xtools at golang.org/x/tools can't be resolved with dep until this work is completed.

It can. Point required at the actual main binary you want, not the root of the project. As the docs note, required specifications are on packages, not projects.

@mcandre
Copy link

mcandre commented Feb 21, 2018

@sdboyer When I point required at the full path to a binary, then dep ensure complains that the path should be shortened to just the repository top level.

@szobov
Copy link

szobov commented Jul 23, 2018

Hi there!
Thanks @carolynvs for this great job.
But, what can I do to help? I think it's very important feature in we need this to be in done.

Best regards.

@carolynvs
Copy link
Collaborator Author

Let me rebase and see if I can get a review of my changes. I wasn't confident that this is "the right way" to solve it, which is why it's been sitting for so long.

@sdboyer
Copy link
Member

sdboyer commented Jul 23, 2018

@carolynvs oooh, i actually had some more thoughts about this.

Instead of imbuing required with special properties, it might be better if we just introduce a new property into Gopkg.toml, something like allow-errs. This would allow the user to ignore type/missing/whatever errors from any path, not just required ones. The logic to manage this in the solver would be a bit less complicated, because we don't have to worry about keeping track of whether a given import path is required, imported, or both - we just have the map of the paths at which we allow errs, period.

This would have some ignored-like behaviors, so we'd want to make sure to carefully document it.

@carolynvs does that help with your (un)certainty at all?

@carolynvs
Copy link
Collaborator Author

OMG that is waaaaaay better! 馃挴 I'm going to close this PR and open a new one with that behavior instead.

@carolynvs carolynvs closed this Jul 23, 2018
@cove
Copy link

cove commented Nov 30, 2018

OMG that is waaaaaay better! 馃挴 I'm going to close this PR and open a new one with that behavior instead.

hey @carolynvs did you end up opening a PR?

@carolynvs
Copy link
Collaborator Author

@cove No, sorry to say that I don't have time to work on dep anymore all things considered. 馃槩

@szobov
Copy link

szobov commented Dec 1, 2018

So, do we have someone who'll continue work on this feature?

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

Successfully merging this pull request may close these issues.

dep cannot vendor dependencies that do not have Go source code
7 participants