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

[WIP] stack freeze command #4220

Merged
merged 6 commits into from
Aug 21, 2018
Merged

[WIP] stack freeze command #4220

merged 6 commits into from
Aug 21, 2018

Conversation

qrilka
Copy link
Contributor

@qrilka qrilka commented Aug 10, 2018

Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

Please also shortly describe how you tested your change:

  • Integration test in test/integration/tests/4220-freeze-command
  • run stack freeze on my xlsx package

@qrilka qrilka requested a review from snoyberg August 10, 2018 13:36
@qrilka
Copy link
Contributor Author

qrilka commented Aug 10, 2018

@snoyberg could you have a glance on this initial attempt?
Should I add changelog entry in this PR? And what about documentation?
I guess some test is required?

@snoyberg
Copy link
Contributor

could you have a glance on this initial attempt?

Reviewing now, I'll leave comments inline

Should I add changelog entry in this PR? And what about documentation?

Yes and yes. And that reminds me that I need to add an issue about fixing up all the documentation on the pantry branch.

I guess some test is required?

That would be ideal, yes. Ping me if you'd like some help on how to use the integration test suite.

@@ -63,6 +63,7 @@ import Stack.Dot
import Stack.GhcPkg (findGhcPkgField)
import qualified Stack.Nix as Nix
import Stack.FileWatch
import Stack.Freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to git add the new modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch, forgot the main part of it

}

freeze :: HasEnvConfig env => FreezeOpts -> RIO env ()
freeze freezeOpts = do
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of working at the fully-parsed level of a source map, it would be better to work on the original stack.yaml file and any custom snapshots it refers to. I can step through that with you on a call if that would be helpful.

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 just couldn't find that yet and checking out code of stack dot was easy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After having a testcase changing implementation should be even easier

@qrilka
Copy link
Contributor Author

qrilka commented Aug 16, 2018

@snoyberg please check out new version including a test. Will add documentation file a bit later.

@qrilka
Copy link
Contributor Author

qrilka commented Aug 16, 2018

This version outputs pinned version of stack.yaml or snapshot.yaml in case if there are any unpinned dependencies. Maybe it's better to just make it stateless and produce output in any case? Empty output in very confusing. Or we could report something like "It is fine" 🔥 🐕 in this case

plm@(PLMutable _) -> pure plm
resolver' <- completeSnapshotLocation resolver
deps' <- mapM completePackageLocation' deps
when (deps' /= deps || resolver' /= resolver) $
Copy link
Contributor

Choose a reason for hiding this comment

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

How about outputting a message to the user that no freezing was necessary because they're unmodified?

liftIO $ B.putStr $ Yaml.encode p{ projectDependencies = deps'
, projectResolver = resolver'
}
Nothing -> pure ()
Copy link
Contributor

Choose a reason for hiding this comment

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

This deserves at least a logWarn that there was no project found.

when (snap' /= snap) $
liftIO $ B.putStr $ Yaml.encode snap'
Nothing ->
return ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with a logWarn here

, ["name" .= snapshotName snap]
, ["packages" .= map mkUnresolvedPackageLocationImmutable (snapshotLocations snap)]
, if Set.null (snapshotDropPackages snap) then [] else ["drop-packages" .= Set.map CabalString (snapshotDropPackages snap)]
, if Map.null (snapshotFlags snap) then [] else ["flags" .= fmap toCabalStringMap (toCabalStringMap (snapshotFlags snap))]
, if Map.null (snapshotHidden snap) then [] else ["hidden" .= toCabalStringMap (snapshotHidden snap)]
, if Map.null (snapshotGhcOptions snap) then [] else ["ghc-options" .= toCabalStringMap (snapshotGhcOptions snap)]
]
where
(usl, compiler) = unresolveSnapshotLocation $ snapshotParent snap
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good call

main = do
stackCheckStdout ["freeze"] $ \stdOut -> do
let expected = unlines
[ "packages:"
Copy link
Contributor

Choose a reason for hiding this comment

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

No problem with this approach, but you can sort of get multiline strings in Haskell with:

"foo\n\
\bar\n\
\baz"

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 know but unlines looks a bit more readable in this case imo

packages:
- .
extra-deps:
- a50-0.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include a @rev: or similar here, otherwise a Hackage trustee could break this test by making a revision.

@qrilka
Copy link
Contributor Author

qrilka commented Aug 21, 2018

@snoyberg I've resolved conflicts, added more output and also wrote down a basic doc. Please take a look

@snoyberg snoyberg merged commit 7f50951 into commercialhaskell:pantry Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants