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

Adds new-update #4809

Merged
merged 7 commits into from
Oct 26, 2017
Merged

Adds new-update #4809

merged 7 commits into from
Oct 26, 2017

Conversation

angerman
Copy link
Collaborator

@angerman angerman commented Oct 3, 2017

new-update uses the new-style logic to update the repositories. As such it
respects repository fields in the cabal.project(.local) file and updates
them as well. This is essential when working with hackage overlays, where
the overlay repositories are specified as repository fields in the
cabal.project(.local) file.

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

@angerman angerman requested review from ezyang and hvr October 3, 2017 06:55
@angerman
Copy link
Collaborator Author

angerman commented Oct 3, 2017

This is basically a merge of the CmdFreeze.hs (new-freeze) and old Update.hs (update) files.

The command line options are not correct, so if someone could provide some guidance for those, I'd be happy to fix those properly.

@angerman
Copy link
Collaborator Author

@hvr, @23Skidoo can I get a review of this?

new-update uses the new-style logic to update the repositories.  As such it
respects `repository` fields in the `cabal.project(.local)` file and updates
them as well.  This is essential when working with hackage overlays, where
the overlay repositories are specified as `repository` fields in the
`cabal.project(.local)` file.
@angerman
Copy link
Collaborator Author

This still does not properly respect the index state, as I have no idea how to thread that through properly.

However it does everything else as needed. Unless the index-state threading is trivial, I'd prefer that to be a separate PR.

@hvr
Copy link
Member

hvr commented Oct 22, 2017

@angerman I agree; taking into account the index-state business will require more thought (and likely a departure from the old-update UI). Let's get this PR wrapped up; and do refinements incrementally in future PRs.


PS: have you seen the failure

Distribution\Client\CmdUpdate.hs:76:5: warning: [-Wunused-top-binds]
    Defined but not used: `updateRequestRepoState'
<no location info>: error: 
Failing due to -Werror.

?

@angerman
Copy link
Collaborator Author

@hvr, yea, that's just plain stupid.

I've introduced a new datatype

data UpdateRequest = UpdateRequest
   { updateRequestRepoName :: String
   , updateRequestRepoState :: IndexState
   } deriving (Show)

and because I have not yet used updateRequestRepoState, this fails.

What's the proper cabal approved fix for this?

@hvr
Copy link
Member

hvr commented Oct 22, 2017

@angerman I'd just prefix it by _ temporarily... it's not like this is a public API anyway

@angerman
Copy link
Collaborator Author

Can we merge now?

updateCommand = Client.installCommand {
commandName = "new-update",
commandSynopsis = "Updates list of known packages.",
commandUsage = usageAlternatives "new-update" [ "[FLAGS]" ],
Copy link
Member

Choose a reason for hiding this comment

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

I think [FLAGS] should reflect that we may also supply a list of repo labels. C.f.

Usage: cabal new-build [TARGETS] [FLAGS]

vs.

Usage: cabal new-update [FLAGS]

which would be better as

Usage: cabal new-update [FLAGS] [REPOS]

commandNotes = Just $ \pname ->
"Examples:\n"
++ " " ++ pname ++ " new-update\n"
++ " Download the package list for all known remote repositories.\n\n"
Copy link
Member

@hvr hvr Oct 25, 2017

Choose a reason for hiding this comment

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

We may want to add an example here showing how to update a single repo

and we may point out that a TARGET has the format <repo-id>[,<index-state>] where <index-state> is the same syntax as supported by the --index-state flag which currently is:

    --index-state=STATE              Use source package index state as it
                                     existed at a previous time. Accepts
                                     unix-timestamps (e.g. '@1474732068'),
                                     ISO8601 UTC timestamps (e.g.
                                     '2016-09-24T17:47:48Z'), or 'HEAD'
                                     (default: 'HEAD').

consequently, the following invocations are all valid (I tested them w/o your patch):

cabal new-update hackage.haskell.org,@1474732068
cabal new-update hackage.haskell.org,2016-09-24T17:47:48Z
cabal new-update hackage.haskell.org,HEAD
cabal new-update hackage.haskell.org

current_ts <- currentIndexTimestamp (lessVerbose verbosity) repoCtxt repo
-- NB: always update the timestamp, even if we didn't actually
-- download anything
writeIndexTimestamp index (fromFlag (updateIndexState updateFlags))
Copy link
Member

Choose a reason for hiding this comment

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

So that's the call where we'd need to propagate the <index-state> value to... what's the problem? :-)

Copy link
Member

@hvr hvr Oct 25, 2017

Choose a reason for hiding this comment

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

@angerman just to make this more explicit:

As you can see, writeIndexTimestamp writes the 01-index.timestamp file which is stored alongside each 01-index.tar in the package/repo cache:

-- | Write the 'IndexState' to the filesystem
writeIndexTimestamp :: Index -> IndexState -> IO () 
writeIndexTimestamp index st
  = writeFile (timestampFile index) (display st)

so all you need to do, is pass writeIndexTimestamp the optional [,<index-state>] argument associated with that specific repository's index.

@angerman
Copy link
Collaborator Author

Let's do this!

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.

2 participants