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

[WIP] dep: Enable importing other tools and add integration tests #1277

Merged
merged 5 commits into from
Nov 10, 2017

Conversation

ChinmayR
Copy link
Contributor

  • Turned off skip tools flag by default
  • Added glide integration tests for the init case
  • Move removing transitive dependencies from the imported manifest to
    finalize manifest stage to avoid ignoring some constraints when
    importing metadata from other tools

What does this do / why do we need it?

Enables dep init to pull data from other tools, not only from direct but also transitive dependencies.

What should your reviewer look out for in this PR?

Moving a.removeTransitiveDependencies(...) from import manifest to finalize manifest. The trans-trans-conflict test fails without this change.

Do you need help or clarification on anything?

Is this a valid approach or should we be adding imported transitive constraints as overrides in the root manifest?

Which issue(s) does this PR fix?

Fixes the dep init part, still need to tackle ensure.
fixes #821
fixes #920

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@ChinmayR
Copy link
Contributor Author

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@ChinmayR ChinmayR force-pushed the import-glide branch 2 times, most recently from b35247a to 23565ae Compare October 18, 2017 18:59
@uberbits
Copy link

uberbits commented Oct 18, 2017

@jmank88 it seems that TestGitSourceListVersionsNoDupes test is a flaky test due to git/github reliability issues. What is your recommendation to go about fixing it?

57bcc65#diff-e5840bdf34ab76e533df859b43e25eb3

Here is the log: https://ci.appveyor.com/project/golang/dep/build/2399

@jmank88
Copy link
Collaborator

jmank88 commented Oct 18, 2017

We generally just re-run the CI when a failure like that occurs, but this appears to have resolved itself for now.

@ChinmayR
Copy link
Contributor Author

Hi @carolynvs , I wanted your input on whether or not this is the best way to solve this. I am afraid this might be going against how preferred versions work but I do not understand them enough to say for sure.

I was running into the same issue as this: #920 (review)

And solved it by not removing transitive dependencies when we import the manifest, but instead removing them during the finalize manifest step.

CHANGELOG.md Outdated
@@ -40,7 +40,7 @@ WIP:

* gps: Process canonical import paths. (#1017)
* gps: Persistent cache. (#1127, #1215)

* dep: Enable importing other tools and add integration tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

We just released v0.3.2. Please move this under v0.3.3.
And also add the PR #. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR with the changes.

@darkowlzz darkowlzz added this to the v0.3.3 milestone Oct 20, 2017
- Turned off skip tools flag by default
- Added glide integration tests for the init case
- Move removing transitive dependencies from the imported manifest to
finalize manifest stage to avoid ignoring some constraints when
importing metadata from other tools
@uberbits
Copy link

@carolynvs is there any chance you can review this diff? Thanks!

@carolynvs
Copy link
Collaborator

@ChinmayR Apologies for my slow response! I will have time to do a review tomorrow.

Also I talked with the other maintainers yesterday and created an epic (#1335) to cover all the changes that need to be made before we can "flip the switch" on importing during solve (as opposed to only during init).

Once this is ready, what I will probably end up doing is merging but not turning on the feature immediately, not until the other "safety features" are in as well.

@carolynvs
Copy link
Collaborator

@ChinmayR I won't have a chance to review until next week sorry! Life came up. 😞

@carolynvs carolynvs changed the base branch from master to import-during-solve November 8, 2017 15:01
Copy link
Collaborator

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Sorry this took so long!!! You've done a ton of great work that is setting us up well for the rest of this epic (#1335). So even though I may shuffle your test data around, it's really useful. Thank you for the extensive tests! 😀

@@ -165,6 +164,11 @@ func (a *rootAnalyzer) DeriveManifestAndLock(dir string, pr gps.ProjectRoot) (gp
}

func (a *rootAnalyzer) FinalizeRootManifestAndLock(m *dep.Manifest, l *dep.Lock, ol dep.Lock) {
// Transitive dependencies could sneak into the manifest when other importers are used
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is another open issue to appropriately handle imported constraints during solve: #1316. Would you please back out this change?

Since #1316 is going to impact many of your test cases, and you have a great set of data, if backing out this change breaks something, we can review and disable temporarily until #1316 is in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are looking to migrate folks over from glide to dep internally asap. I will remove it from this PR and we will use this code internally for now until #1316 is done. Please let me know if I can help in any way to expedite that issue. We can discuss how we want to tackle it and perhaps I can help with the changes?

I will add a comment of our need in #1316 , which is basically also needing to import transient dependencies of the root repo and add them to constraints to enable correct resolving.

hash: 16053c82a71f9bd509b05a4523df6bc418aed2083e4b8bd97a870bbc003256f8
updated: 2017-03-07T17:02:32.214383898-06:00
imports:
- name: github.com/ChinmayR/deptestglideA
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, after this is merged into the feature branch, I am going to fork your test repo(s) and switch dep to point to that instead. dep has a policy of only relying on external repos that are managed by one of the maintainers, so that once this is in, you won't be on the hook to keep around the repos.

Thank you for building up a good set of test data! ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good 👍

"commands": [
["init", "-no-examples", "-v"]
],
"error-expected": "v0.3.0: unable to parse"
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, I am going to keep this test case for now but once #1315 (which is also part of the epic) is implemented, it will probably be removed because the behavior is changing to not hard fail during solve. The testdata in your repo will be kept and used though, thank you for building that out! ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1 @@
Take a direct dependency where the version is defined by glide.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this since the scenario is already covered by glide/case1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed that, will remove it.

@@ -0,0 +1 @@
Take a direct dependency on a transient dependency where the versions are conflicted. Resolving should fail.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this test starts failing after the other changes I have requested are made, let's ignore it test temporarily ( add a .ignore file extension to the testcase.json file). The scenario may require additional handling and will be addressed in another issue: #1316.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test does not fail since the existing code only removes transient dependencies read from importers. Since there is also a direct dependency here, the test passes.

CHANGELOG.md Outdated
@@ -4,6 +4,10 @@ BUG FIXES:

* Releases targeting Windows now have a `.exe` suffix (#1291).

WIP:

* dep: Enable importing other tools and add integration tests (#1277)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change this to Enable importing external configuration from dependencies during init, so that the scope of the change is more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a better comment, updated it.

@@ -0,0 +1 @@
Take a direct dependency on a transient dependency where the versions are not conflicted. Resolving should pass.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this test starts failing after the other changes I have requested are made, let's ignore it test temporarily ( add a .ignore file extension to the testcase.json file). The scenario may require additional handling and will be addressed in another issue: #1316.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test does not fail since the existing code only removes transient dependencies read from importers. Since there is also a direct dependency here, the test passes.

@@ -0,0 +1 @@
Have two transient dependencies have different versions of the same repo. Resolving should fail.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is asserting an unwanted behavior that will be addressed in #1316. But it's a useful test case for that other story, so let's not remove it. Would you please add a .ignore to the testcase.json, and when we implement the other issue, we'll turn this test on, with the correct expected results (i.e. it should not fail).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test starts to fail since we do not import transient dependencies as constraints of the root repo. This results in incorrect resolving since only constraints that are imported are of direct dependencies of the root. I will add a comment to #1316 to also handle this case explicitly. Shouldn't the resolving for this test fail when the test case is enabled again as part of #1316 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe preferred versions handle this case but currently we have no idea of dealing with conflicting preferred versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm a bit fuzzy off the top of my head on the topic of conflicting preferred versions. 😊 But that is a dep-wide question, not just for import, so I'd rather deal with it in that other issue and rope in Sam then.

@@ -14,8 +14,8 @@
version = "v0.8.1"

[solve-meta]
analyzer-name = "dep+import"
analyzer-name = "dep"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doh! I wonder why this didn't get picked up sooner? Thanks for fixing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@carolynvs carolynvs removed the request for review from sdboyer November 8, 2017 18:57
@ChinmayR
Copy link
Contributor Author

ChinmayR commented Nov 9, 2017

Thank you taking a look at this @carolynvs , glad to have it moving forward. I addressed the PR comments with some open questions.

We are looking to migrate folks over from glide to dep internally asap and #1316 is a hard blocker for us. I removed the fix from this PR but we will use this code internally for now until #1316 is done. Please let me know if I can help in any way to expedite that issue. We can discuss how we want to tackle it and perhaps I can help with the changes?

@carolynvs
Copy link
Collaborator

tip is failing but that is not related to this PR, and we are seeing that elsewhere. I would love help with getting this feature pushed through, most especially on the test and scenario identification end. I am starting on #1316 this weekend and we can build from there! ❤️

@carolynvs carolynvs merged commit e1f5c5f into golang:import-during-solve Nov 10, 2017
@uberbits
Copy link

@ChinmayR congrats! on your first accepted commit.

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.

6 participants