Skip to content
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

Improve and expose fetchRepos/fetchReposRaw #41

Merged

Conversation

hasufell
Copy link
Contributor

Necessary for commercialhaskell/stack#5411

Please review this carefully. I don't understand most of pantry, but this seemed like the right place to inject and my manual tests succeeded.

Stack will then run fetchReposRaw at a specific place for pre-fetching. So we don't really change existing API.

src/Pantry/Repo.hs Outdated Show resolved Hide resolved
@hasufell hasufell force-pushed the hasufell/PR/extra-deps-cloning branch from 306dfac to 9ecd1ba Compare October 1, 2021 11:46
@hasufell hasufell marked this pull request as draft October 1, 2021 11:53
@hasufell hasufell changed the title Improve and expose fetchRepos/fetchReposRaw [WIP] Improve and expose fetchRepos/fetchReposRaw Oct 1, 2021
@hasufell hasufell force-pushed the hasufell/PR/extra-deps-cloning branch from 9ecd1ba to 9bbcdb1 Compare October 1, 2021 12:00
@hasufell hasufell marked this pull request as ready for review October 1, 2021 12:00
@hasufell hasufell changed the title [WIP] Improve and expose fetchRepos/fetchReposRaw Improve and expose fetchRepos/fetchReposRaw Oct 1, 2021
@hasufell
Copy link
Contributor Author

hasufell commented Oct 1, 2021

One thing this doesn't solve is this: if you add a new subdirectory to an exising git extra-dep, it will have to clone once more. I'm not sure if there's a way around that.

Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

The overall logic looks really solid, impressive work here given your first work on this code base, thank you! I've left some comments inline.

src/Pantry/Repo.hs Outdated Show resolved Hide resolved
src/Pantry/Repo.hs Outdated Show resolved Hide resolved
src/Pantry/Types.hs Outdated Show resolved Hide resolved
src/Pantry/Repo.hs Show resolved Hide resolved
src/Pantry/Repo.hs Outdated Show resolved Hide resolved
src/Pantry/Repo.hs Outdated Show resolved Hide resolved
@hasufell hasufell force-pushed the hasufell/PR/extra-deps-cloning branch from 9bbcdb1 to fc7de76 Compare October 5, 2021 11:53
@hasufell hasufell requested a review from snoyberg October 5, 2021 11:54
Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Looks great, one minor comment for consideration. I'm OK merging as is though, so let me know whether you think that change is worth it or not. If not, I'll merge/release in the current state.

where
withCache inner = do
pkgs <- forM aRepoSubdirs $ \(subdir, rpm) -> withStorage $ do
loadRepoCache (Repo aRepoUrl aRepoCommit aRepoType subdir) subdir >>= \case
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

-- | Repository without subdirectory information.
--
-- @since 0.5.3
data SimpleRepo = SimpleRepo
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

rToSimpleRepo Repo {..} = SimpleRepo { sRepoUrl = repoUrl, sRepoCommit = repoCommit, sRepoType = repoType }

data AggregateRepo = AggregateRepo
{ aRepoUrl :: !Text
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a strong recommendation, but what about using SimpleRepo here instead of duplicating the fields?

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'll give it a shot

@hasufell hasufell force-pushed the hasufell/PR/extra-deps-cloning branch from 68e989e to 60ca785 Compare October 5, 2021 14:33
Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

LGTM!

@snoyberg snoyberg merged commit 50c77d5 into commercialhaskell:master Oct 5, 2021
@hasufell
Copy link
Contributor Author

hasufell commented Oct 5, 2021

@snoyberg thanks, the next step would be the actual stack PR that calls fetchReposRaw at the appropriate place: commercialhaskell/stack#5624

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

Successfully merging this pull request may close these issues.

2 participants