Improved VCS support #5333
Improved VCS support #5333
Conversation
This patch covers a new VCS abstraction with corresponding tests. It also re-implements the get command in terms of the VCS and adds unit tests (including optional network tests). This does slightly change the behaviour of the get command: instead of looking for which VCSs are available on the system and then choosing which repo to try and get, we instead pick a preferred repo and then we look for the corresponding VCS and fail if it's not available. In practice this difference does not matter because we don't have multiple repos for the same package using different VCSs.
| RepoThis -> 0 | ||
| RepoHead -> case repoTag r of | ||
| -- If the type is 'head' but the author specified a tag, they | ||
| -- probably meant to create a 'this' repository but screwed up. |
There was a problem hiding this comment.
Not entirely sure how ideal it is to help people write incorrect project configs.
There was a problem hiding this comment.
I'm not going to defend it, this is just preserving the existing behaviour. This bit of code was just moved from the existing Get module.
There was a problem hiding this comment.
Okay, that's fair. I'd personally lean towards changing it when we're poking around here, but 🤷♀️.
| --TODO: export this from Distribution.Simple.Program.Db | ||
| configurePrograms :: [Program] -> IO ProgramDb | ||
| configurePrograms = foldM (flip (configureProgram verbosity)) emptyProgramDb | ||
| -} |
There was a problem hiding this comment.
Why is this commented out?
| ] | ||
|
|
||
| newtype OptionShowSolverLog = OptionShowSolverLog Bool | ||
| deriving Typeable | ||
|
|
||
| instance IsOption OptionShowSolverLog where | ||
| defaultValue = OptionShowSolverLog False | ||
| parseValue = fmap OptionShowSolverLog . safeRead | ||
| parseValue = fmap OptionShowSolverLog . safeReadBool |
There was a problem hiding this comment.
I guess, but it keeps consistency between OptionShowSolverLog and RunNetworkTests which are both bool type options.
There was a problem hiding this comment.
Yeah, I wasn't saying "take it out", I just wanted to make sure there wasn't some weird thing I was missing about the change. 🙂
| srcurl <- repoLocation repo | ||
| vcs <- Map.lookup rtype vcss | ||
| return (vcsCloneRepo vcs (vcsProgram vcs) repo srcurl destdir) | ||
| -} |
There was a problem hiding this comment.
Why is this commented out?
| return $ BranchCmd $ \verbosity dst -> do | ||
| notice verbosity ("svn: checkout " ++ show src) | ||
| rawSystemExitCode verbosity "svn" (args dst) | ||
| forkPackages :: Verbosity |
There was a problem hiding this comment.
This whole chunk seems to be very similar to what will be needed to actually fix #2189, so I don't know if it should be 'owned' by .Get. I'd probably put it with the other VCS code.
There was a problem hiding this comment.
Actually, new-build is a different problem. Here in forkPackages we already have [SourcePackage loc] which indicate which packages (e.g. via package name and package description) come from this repository.
In new-build you have the repository but no clue which packages it contains. You have to clone the repo first and then discover the packages. So I think forkPackages is fine here as new-build needs different codepath anyways.
There was a problem hiding this comment.
Oh, that makes perfect sense. 🤦🏻♀️
|
This is something that I want to be finished ASAP. How can I help get it ready to merge? Failing that, I'm not sure on the etiquette involved in working on a patch that's dependent on an as-of-yet unmerged change. |
|
Noted. I'm working on the integration parts and then plan to go back and tidy everything up (e.g. delete unused commented out bits, make a series of comprehensible patches for merging). |
NOTE: This is a commit by @dcoutts ; I'm turning this into a PR as this is becoming GSOC-relevant, and so that @typedrat (and others) can review it, and eventually merge it.
This patch covers a new VCS abstraction with corresponding tests. It
also re-implements the get command in terms of the VCS and adds unit
tests (including optional network tests).
This does slightly change the behaviour of the get command: instead of
looking for which VCSs are available on the system and then choosing
which repo to try and get, we instead pick a preferred repo and then we
look for the corresponding VCS and fail if it's not available. In
practice this difference does not matter because we don't have multiple
repos for the same package using different VCSs.
Please include the following checklist in your PR:
[ci skip]is used to avoid triggering the build bots.Please also shortly describe how you tested your change. Bonus points for added tests!