New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal/gps: ensure packages are deducible before attempting to solve #697

Merged
merged 3 commits into from Aug 5, 2017

Conversation

Projects
None yet
3 participants
@ibrasho
Copy link
Collaborator

ibrasho commented Jun 1, 2017

An attempt to resolve #581.

I still think it could use some polishing, but I wanted to check if I'm moving in the right direction.

@googlebot googlebot added the cla: yes label Jun 1, 2017

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch 2 times, most recently from bbde9bb to da33cb1 Jun 1, 2017

@sdboyer
Copy link
Member

sdboyer left a comment

Yep! Definitely the right direction.

One thing, though. I realize I'm countermanding myself, but let's actually do this under solver.Solve() instead of Prepare(). These deductions have the possibility of triggering network activity, and I'd prefer to keep the work done in Prepare() strictly IO-free.

It'd be fine to add a solver.validateParams() method that does this, and call it nearly first thing, immediately before solver.selectRoot() in solver.Solve(). Chances are we'll have some more validation like this to add, and we can lump it into that method.

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Jun 1, 2017

@sdboyer Done. Note that Prepare() doc says the following:

// This function reads and validates the provided SolveParameters. If a problem
// with the inputs is detected, an error is returned.
Otherwise, a Solver is
// returned, ready to hash and check inputs or perform a solving run.

I've extracted the logic to a solver. validateParams() method. But I think it belongs to Prepare(), or that the comment should be clarified.

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch 2 times, most recently from c7a2389 to e159914 Jun 1, 2017

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Jun 1, 2017

@sdboyer It seems that bridge.DeduceProjectRoot() allows race conditions.
solver.mtr is not protected against simultaneous writes by multiple goroutines, and it's not possible to get the gps.SourceManager directly to avoid using bridge.DeduceProjectRoot().

I tried to go back to Prepare(), but noticed that passing arbitrary packages to Prepare in tests fails.

Any preference on how to tackle this?
Adding a mutex over Solver.mtr in bridge.DeduceProjectRoot() will render goroutines useless since it will force sequential execution. I'm thinking of exposing the gps.SourceManager to the gps.solver.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jun 2, 2017

@ibrasho ahh...crap.

Yes, the bridge allows races, because the entire solver is designed to be strictly single-threaded.

OK, given these issues, yes - let's move the verification back to Prepare(). I'm confused by that link you provide, though - I don't see any failures there?

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Jun 2, 2017

@sdboyer That test case adds 2 dependencies called baz and quz. And since they don't exist, Prepare() will return an error and the test will fail. In TestHashInputsOverrides, c is used multiple times too, causing failures.

This is what's causing CI builds to fail.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jun 2, 2017

🤦‍♂️

sorry, of course, that make sense.

let me ponder a bit on the least invasive way to fix this.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jun 2, 2017

OK, new plan. We don't actually need to do this validation within the solver, because it's all failures we'd arrive at eventually, anyway. That's a strong indication that this isn't actually "validation" in the sense that these conditions must be met for solver behavior to be defined. It's actually just preemptively doing work the solver would do later.

Let's do this validation as a standalone function in gps, and call that function directly from dep, prior to Solve()ing. That'll also give us more reusability, which we'll probably need to employ later within sourceGateway.getManifestAndLock(). We need to guard against this there, too - but in a separate PR.

Returning to the original panic encountered in #581, we can do yet another separate PR which is about gps' purely internal graceful handling of that case.

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch 2 times, most recently from b89b600 to 2058950 Jun 3, 2017

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Jun 5, 2017

@sdboyer: This is passing now. But I still don't like it.

I'm creating another instance of gps.rootdata since it's not exported in the solver created by gps.Prepare().

@sdboyer
Copy link
Member

sdboyer left a comment

sorry, i know i've been slow the last week - trying to catch up!

}

// ValidateParams validates the solver parameters to ensure solving can be completed.
func ValidateParams(params SolveParameters, sm SourceManager) error {

This comment has been minimized.

@sdboyer

sdboyer Jun 14, 2017

Member

Need just a basic test for this - feed it some SolveParameters with both valid and invalid import paths.

This comment has been minimized.

@ibrasho

ibrasho Jul 16, 2017

Collaborator

Are the tests in solver_test.go enough?

This comment has been minimized.

@sdboyer

sdboyer Jul 19, 2017

Member

sorry, yes, they are. but, how about we rename that file to solver_inputs_test.go, and move TestBadSolveOpts into there?

This comment has been minimized.

@ibrasho

ibrasho Jul 21, 2017

Collaborator

done.

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch from fc84b7b to f1d5b48 Jun 15, 2017

internal/gps: ensure packages are deducible before attempting to solve
Add gps.ValidateParams to ensure all packages in SolverParams are
deducible.

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch from f1d5b48 to a961d76 Jul 16, 2017

@sdboyer sdboyer referenced this pull request Jul 18, 2017

Closed

dep double clones goconvey #843

@sdboyer sdboyer closed this Jul 19, 2017

@sdboyer sdboyer reopened this Jul 19, 2017

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch 2 times, most recently from e8d34d0 to a961d76 Jul 21, 2017

rename internal/gps/solver_test.go -> internal/gps/solver_inputs_test.go
Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

@sdboyer sdboyer reopened this Jul 26, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 26, 2017

i think the CLA issue should clear if a new commit comes in. maybe just rebase this and re-push, that should be enough.

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch from 1f42a17 to b6260d6 Jul 27, 2017

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Jul 27, 2017

That trick actually works. 😁

@sdboyer
Copy link
Member

sdboyer left a comment

little nit, looks like a debugging fmt.Println() survived

}

deducePkg := func(ip string, sm SourceManager) {
fmt.Println(ip)

This comment has been minimized.

@sdboyer

sdboyer Jul 28, 2017

Member

seems like a debugging print?

gps: move TestBadSolveOpts to solver_inputs_test.go
Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch from b6260d6 to 92df3dc Jul 29, 2017

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Aug 4, 2017

Anything else needed here? 😁

@sdboyer

sdboyer approved these changes Aug 5, 2017

Copy link
Member

sdboyer left a comment

nothing! in we go 😄

@sdboyer sdboyer merged commit 2e56bed into golang:master Aug 5, 2017

7 checks passed

cla/google All necessary CLAs are signed
codeclimate All good!
Details
codeclimate/coverage 74.8% (+4.7%)
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 5, 2017

see, i said that, but then...yeah, we need this to be expanded, now that there are more places we actually solve/prepare params in ensure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment