Merge pull request #181 from ryantm/ux

improve ux for single update case

README.md

@@ -131,19 +131,18 @@ test a package with one command.
 3. To test your config, try to update a single package, like this:
 
    ```
-   ./result/bin/nixpkgs-update update --dry-run --additional-updates "pkg oldVer newVer update-page"`
+   ./result/bin/nixpkgs-update update "pkg oldVer newVer update-page"`
 
    # Example:
-   ./result/bin/nixpkgs-update update --dry-run --additional-updates "tflint 0.15.0 0.15.1 repology.org"`
+   ./result/bin/nixpkgs-update update "tflint 0.15.0 0.15.1 repology.org"`
    ```
 
    replacing `tflint` with the attribute name of the package you actually want
    to update, and the old version and new version accordingly.
 
-   If this works, you are now setup to hack on `nixpkgs-update`! Since we passed
-   `--dry-run`, it will just print a `diff` of the updated nix expression, then
-   exit. If you run it without `--dry-run`, it will actually send a pull
-   request, which looks like this: https://github.com/NixOS/nixpkgs/pull/82465
+   If this works, you are now setup to hack on `nixpkgs-update`! If
+   you run it with `--pr`, it will actually send a pull request, which
+   looks like this: https://github.com/NixOS/nixpkgs/pull/82465
 
 
 4. If you'd like to send a batch of updates, get a list of outdated packages and

app/Main.hs

@@ -16,22 +16,21 @@ import OurPrelude
 import qualified Repology
 import System.IO (BufferMode (..), hSetBuffering, stderr, stdout)
 import qualified System.Posix.Env as P
-import Update (cveAll, cveReport, sourceGithubAll, updateAll)
+import Update (cveAll, cveReport, sourceGithubAll, updateAll, updatePackage)
 import Utils (Options (..), UpdateEnv (..), getGithubToken, setupNixpkgs)
 
 default (T.Text)
 
 data UpdateOptions
   = UpdateOptions
-      { dry :: Bool,
+      { pr :: Bool,
         cachix :: Bool,
-        additionalUpdates :: Text,
         outpaths :: Bool
       }
 
 data Command
   = UpdateList UpdateOptions
-  | Update UpdateOptions
+  | Update UpdateOptions Text
   | DeleteDone
   | Version
   | UpdateVulnDB
@@ -43,15 +42,16 @@ data Command
 updateOptionsParser :: O.Parser UpdateOptions
 updateOptionsParser =
   UpdateOptions
-    <$> O.switch
-      ( O.long "dry-run"
-          <> O.help
-            "Do everything except actually pushing the updates to the remote repository"
-      )
+    <$> O.flag False True (O.long "pr" <> O.help "Make a pull request using Hub.")
     <*> O.flag False True (O.long "cachix" <> O.help "Push changes to Cachix")
-    <*> O.strOption (O.long "additional-updates" <> O.help "A string of updates formatted the same way as packages-to-update.txt" <> O.value "")
     <*> O.flag False True (O.long "outpaths" <> O.help "Calculate outpaths to determine the branch to target")
 
+updateParser :: O.Parser Command
+updateParser =
+  Update
+    <$> updateOptionsParser
+    <*> O.strArgument (O.metavar "UPDATE_INFO" <> O.help "update string of the form: 'pkg oldVer newVer update-page'\n\n example: 'tflint 0.15.0 0.15.1 repology.org'")
+
 commandParser :: O.Parser Command
 commandParser =
   O.hsubparser
@@ -60,7 +60,7 @@ commandParser =
         (O.info (UpdateList <$> updateOptionsParser) (O.progDesc "Update a list of packages"))
         <> O.command
           "update"
-          (O.info (Update <$> updateOptionsParser) (O.progDesc "Update packages"))
+          (O.info (updateParser) (O.progDesc "Update one package"))
         <> O.command
           "delete-done"
           ( O.info
@@ -124,19 +124,22 @@ main = do
       setupNixpkgs token
       P.setEnv "GITHUB_TOKEN" (T.unpack token) True
       deleteDone token
-    UpdateList UpdateOptions {dry, cachix, additionalUpdates, outpaths} -> do
+    UpdateList UpdateOptions {pr, cachix, outpaths} -> do
       token <- getGithubToken
       updates <- T.readFile "packages-to-update.txt"
       setupNixpkgs token
       P.setEnv "PAGER" "" True
       P.setEnv "GITHUB_TOKEN" (T.unpack token) True
-      updateAll (Options dry token cachix outpaths) (updates <> "\n" <> additionalUpdates)
-    Update UpdateOptions {dry, cachix, additionalUpdates, outpaths} -> do
+      updateAll (Options pr True token cachix outpaths) updates
+    Update UpdateOptions {pr, cachix} update -> do
       token <- getGithubToken
       setupNixpkgs token
       P.setEnv "PAGER" "" True
       P.setEnv "GITHUB_TOKEN" (T.unpack token) True
-      updateAll (Options dry token cachix outpaths) additionalUpdates
+      result <- updatePackage (Options pr False token cachix False) update
+      case result of
+        Left e -> T.putStrLn e
+        Right () -> T.putStrLn "Done."
     Version -> do
       v <- runExceptT Nix.version
       case v of
@@ -146,17 +149,17 @@ main = do
     CheckAllVulnerable -> do
       setupNixpkgs undefined
       updates <- T.readFile "packages-to-update.txt"
-      cveAll (Options undefined undefined undefined undefined) updates
+      cveAll (Options undefined undefined undefined undefined undefined) updates
     CheckVulnerable productID oldVersion newVersion -> do
       setupNixpkgs undefined
       report <-
         cveReport
-          (UpdateEnv productID oldVersion newVersion Nothing (Options False undefined False False))
+          (UpdateEnv productID oldVersion newVersion Nothing (Options False False undefined False False))
       T.putStrLn report
     SourceGithub -> do
       token <- getGithubToken
       updates <- T.readFile "packages-to-update.txt"
       setupNixpkgs token
       P.setEnv "GITHUB_TOKEN" (T.unpack token) True
-      sourceGithubAll (Options False token False False) updates
+      sourceGithubAll (Options False False token False False) updates
     FetchRepology -> Repology.fetch

src/Git.hs

@@ -98,7 +98,7 @@ push updateEnv =
             "origin",
             T.unpack (branchName updateEnv)
           ]
-            ++ ["--dry-run" | dryRun (options updateEnv)]
+            ++ ["--dry-run" | doPR (options updateEnv)]
         )
     )
 

src/Rewrite.hs

@@ -2,6 +2,7 @@
 
 module Rewrite
   ( Args (..),
+    runAll,
     golangModuleVersion,
     quotedUrls,
     quotedUrlsET,
@@ -47,6 +48,14 @@ data Args
         derivationContents :: Text
       }
 
+runAll :: (Text -> IO ()) -> Args -> ExceptT Text IO [Text]
+runAll log rwArgs = do
+  msg1 <- Rewrite.version log rwArgs
+  msg2 <- Rewrite.rustCrateVersion log rwArgs
+  msg3 <- Rewrite.golangModuleVersion log rwArgs
+  msg4 <- Rewrite.quotedUrlsET log rwArgs
+  return $ catMaybes [msg1, msg2, msg3, msg4]
+
 --------------------------------------------------------------------------------
 -- The canonical updater: updates the src attribute and recomputes the sha256
 version :: MonadIO m => (Text -> m ()) -> Args -> ExceptT Text m (Maybe Text)

src/Update.hs

@@ -7,6 +7,7 @@
 
 module Update
   ( updateAll,
+    updatePackage,
     cveReport,
     cveAll,
     sourceGithubAll,
@@ -33,6 +34,7 @@ import OurPrelude
 import Outpaths
 import qualified Rewrite
 import qualified Time
+import Data.Maybe (fromJust)
 import Utils
   ( Options (..),
     URL,
@@ -67,13 +69,22 @@ logFileName = do
   putStrLn ("Using log file: " <> logFile)
   return logFile
 
-updateAll :: Options -> Text -> IO ()
-updateAll o updates = do
+getLog :: Options -> IO (Text -> IO ())
+getLog o = do
+  if batchUpdate o
+    then do
       logFile <- logFileName
       let log = log' logFile
       T.appendFile logFile "\n\n"
+      return log
+    else
+      return T.putStrLn
+
+updateAll :: Options -> Text -> IO ()
+updateAll o updates = do
+  log <- getLog o
   log "New run of nixpkgs-update"
-  when (dryRun o) $ log "Dry Run."
+  when (doPR o) $ log "Will do push to origin and do PR on success."
   when (pushToCachix o) $ log "Will push to cachix."
   when (calculateOutpaths o) $ log "Will calculate outpaths."
   twoHoursAgo <- runM $ Time.runIO Time.twoHoursAgo
@@ -127,9 +138,9 @@ updateLoop o log (Left e : moreUpdates) mergeBaseOutpathsContext = do
   log e
   updateLoop o log moreUpdates mergeBaseOutpathsContext
 updateLoop o log (Right (pName, oldVer, newVer, url) : moreUpdates) mergeBaseOutpathsContext = do
-  log (pName <> " " <> oldVer <> " -> " <> newVer)
+  log (pName <> " " <> oldVer <> " -> " <> newVer <> fromMaybe "" (fmap (" " <>) url))
   let updateEnv = UpdateEnv pName oldVer newVer url o
-  updated <- updatePackage log updateEnv mergeBaseOutpathsContext
+  updated <- updatePackageBatch log updateEnv mergeBaseOutpathsContext
   case updated of
     Left failure -> do
       log $ "FAIL " <> failure
@@ -154,14 +165,14 @@ updateLoop o log (Right (pName, oldVer, newVer, url) : moreUpdates) mergeBaseOut
 -- - the merge base commit (should be updated externally to this function)
 -- - the merge base context should be updated externally to this function
 -- - the commit for branches: master, staging, staging-next, python-unstable
-updatePackage ::
+updatePackageBatch ::
   (Text -> IO ()) ->
   UpdateEnv ->
   IORef MergeBaseOutpathsInfo ->
   IO (Either Text ())
-updatePackage log updateEnv mergeBaseOutpathsContext =
+updatePackageBatch log updateEnv mergeBaseOutpathsContext =
   runExceptT $ do
-    let dry = dryRun . options $ updateEnv
+    let pr = doPR . options $ updateEnv
     --
     -- Filters that don't need git
     Blacklist.packageName (packageName updateEnv)
@@ -169,17 +180,13 @@ updatePackage log updateEnv mergeBaseOutpathsContext =
     --
     -- Update our git checkout
     Git.fetchIfStale <|> liftIO (T.putStrLn "Failed to fetch.")
-    -- If we're doing a dry run, we want to re-run locally even if there's
-    -- already a PR open upstream
-    unless dry $
+    unless pr $
       Git.checkAutoUpdateBranchDoesntExist (packageName updateEnv)
     Git.cleanAndResetTo "master"
     --
     -- Filters: various cases where we shouldn't update the package
     attrPath <- Nix.lookupAttrPath updateEnv
-    -- If we're doing a dry run, we want to re-run locally even if there's
-    -- already a PR open upstream
-    unless dry $
+    unless pr $
       GH.checkExistingUpdatePR updateEnv attrPath
     Blacklist.attrPath attrPath
     Version.assertCompatibleWithPathPin updateEnv attrPath
@@ -223,11 +230,7 @@ updatePackage log updateEnv mergeBaseOutpathsContext =
     -- that we actually should be touching this file. Get to work processing the
     -- various rewrite functions!
     let rwArgs = Rewrite.Args updateEnv attrPath derivationFile derivationContents
-    msg1 <- Rewrite.version log rwArgs
-    msg2 <- Rewrite.rustCrateVersion log rwArgs
-    msg3 <- Rewrite.golangModuleVersion log rwArgs
-    msg4 <- Rewrite.quotedUrlsET log rwArgs
-    let msgs = catMaybes [msg1, msg2, msg3, msg4]
+    msgs <- Rewrite.runAll log rwArgs
     ----------------------------------------------------------------------------
     --
     -- Compute the diff and get updated values
@@ -249,9 +252,8 @@ updatePackage log updateEnv mergeBaseOutpathsContext =
     --
     -- Publish the result
     lift . log $ "Successfully finished processing"
-    unless dry $ do
     result <- Nix.resultLink
-      publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs
+    publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result (Just opDiff) msgs
 
 publishPackage ::
   MonadIO m =>
@@ -261,7 +263,7 @@ publishPackage ::
   Text ->
   Text ->
   Text ->
-  Set ResultLine ->
+  Maybe (Set ResultLine) ->
   [Text] ->
   ExceptT Text m ()
 publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs = do
@@ -303,17 +305,12 @@ publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs = d
   Git.commit commitMsg
   commitHash <- Git.headHash
   -- Try to push it three times
-  Git.push updateEnv <|> Git.push updateEnv <|> Git.push updateEnv
+  when (doPR . options $ updateEnv)
+    (Git.push updateEnv <|> Git.push updateEnv <|> Git.push updateEnv)
   isBroken <- Nix.getIsBroken attrPath
-  lift untilOfBorgFree
-  let base =
-        if numPackageRebuilds opDiff < 100
-          then "master"
-          else "staging"
-  lift $
-    GH.pr
-      base
-      ( prMessage
+  when (batchUpdate . options $ updateEnv)
+    (lift untilOfBorgFree)
+  let prMsg = prMessage
           updateEnv
           isBroken
           metaDescription
@@ -326,10 +323,16 @@ publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs = d
           attrPath
           maintainersCc
           result
-          (outpathReport opDiff)
+          (fromMaybe "" (outpathReport <$> opDiff))
           cveRep
           cachixTestInstructions
-      )
+  if (doPR . options $ updateEnv)
+  then do
+    let base = if (isNothing opDiff || numPackageRebuilds (fromJust opDiff) < 100)
+        then "master"
+        else "staging"
+    lift $ GH.pr base prMsg
+  else liftIO $ T.putStrLn prMsg
   Git.cleanAndResetTo "master"
 
 commitMessage :: UpdateEnv -> Text -> Text
@@ -522,3 +525,48 @@ doCachix log updateEnv resultPath =
     else do
       lift $ log "skipping cachix"
       return "Build yourself:"
+
+updatePackage ::
+  Options ->
+  Text ->
+  IO (Either Text ())
+updatePackage o updateInfo = do
+  runExceptT $ do
+    let (p, oldV, newV, url) = head (rights (parseUpdates updateInfo))
+    let updateEnv = UpdateEnv p oldV newV url o
+    let log = T.putStrLn
+    Nix.assertNewerVersion updateEnv
+    attrPath <- Nix.lookupAttrPath updateEnv
+    Version.assertCompatibleWithPathPin updateEnv attrPath
+    derivationFile <- Nix.getDerivationFile attrPath
+    --
+    -- Get the original values for diffing purposes
+    derivationContents <- liftIO $ T.readFile derivationFile
+    oldHash <- Nix.getOldHash attrPath
+    oldSrcUrl <- Nix.getSrcUrl attrPath
+    --
+    ----------------------------------------------------------------------------
+    -- UPDATES
+    -- At this point, we've stashed the old derivation contents and validated
+    -- that we actually should be touching this file. Get to work processing the
+    -- various rewrite functions!
+    let rwArgs = Rewrite.Args updateEnv attrPath derivationFile derivationContents
+    msgs <- Rewrite.runAll log rwArgs
+    ----------------------------------------------------------------------------
+    --
+    -- Compute the diff and get updated values
+    diffAfterRewrites <- Git.diff
+    lift . log $ "Diff after rewrites:\n" <> diffAfterRewrites
+    updatedDerivationContents <- liftIO $ T.readFile derivationFile
+    newSrcUrl <- Nix.getSrcUrl attrPath
+    newHash <- Nix.getHash attrPath
+    -- Sanity checks to make sure the PR is worth opening
+    when (derivationContents == updatedDerivationContents) $ throwE "No rewrites performed on derivation."
+    when (oldSrcUrl == newSrcUrl) $ throwE "Source url did not change. "
+    when (oldHash == newHash) $ throwE "Hashes equal; no update necessary"
+    Nix.build attrPath
+    --
+    -- Publish the result
+    lift . log $ "Successfully finished processing"
+    result <- Nix.resultLink
+    publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result Nothing msgs

src/Utils.hs

@@ -103,7 +103,8 @@ instance ToField VersionMatcher where
 
 data Options
   = Options
-      { dryRun :: Bool,
+      { doPR :: Bool,
+        batchUpdate :: Bool,
         githubToken :: Text,
         pushToCachix :: Bool,
         calculateOutpaths :: Bool

test/RewriteSpec.hs

@@ -23,7 +23,7 @@ spec = do
     it "quotes an unquoted meta.homepage URL" do
       nixQuotedHomepageBad <- T.readFile "test_data/quoted_homepage_bad.nix"
       nixQuotedHomepageGood <- T.readFile "test_data/quoted_homepage_good.nix"
-      let options = Utils.Options False "" False False
+      let options = Utils.Options False False "" False False
       let updateEnv = Utils.UpdateEnv "inadyn" "2.5" "2.6" Nothing options
       -- TODO test correct file is being read
       let rwArgs = Rewrite.Args updateEnv "inadyn" undefined undefined