From b30faed1b1d5e2a079f4823735656a937e2d0058 Mon Sep 17 00:00:00 2001 From: Ryan Mulligan Date: Sun, 5 Apr 2020 17:23:12 -0700 Subject: [PATCH 1/3] improve ux for single update case * instead of --dry-run use --pr to indicate a PR is wanted * instead of additional updates, have update take the update as an argument * print out stdout instead of log file in the case of non-batch updates * print PR message to log even in the case of no PR --- README.md | 11 +++++----- app/Main.hs | 34 +++++++++++++++---------------- src/Git.hs | 2 +- src/Update.hs | 49 +++++++++++++++++++++++++-------------------- src/Utils.hs | 3 ++- test/RewriteSpec.hs | 2 +- 6 files changed, 53 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index d5a0859..e3307be 100644 --- a/README.md +++ b/README.md @@ -128,19 +128,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 diff --git a/app/Main.hs b/app/Main.hs index 1071630..f84ea91 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -23,15 +23,14 @@ 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,19 @@ 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, outpaths} 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 + updateAll (Options pr False token cachix outpaths) update Version -> do v <- runExceptT Nix.version case v of @@ -146,17 +146,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 diff --git a/src/Git.hs b/src/Git.hs index 43693e2..89776d8 100644 --- a/src/Git.hs +++ b/src/Git.hs @@ -98,7 +98,7 @@ push updateEnv = "origin", T.unpack (branchName updateEnv) ] - ++ ["--dry-run" | dryRun (options updateEnv)] + ++ ["--dry-run" | doPR (options updateEnv)] ) ) diff --git a/src/Update.hs b/src/Update.hs index 29a8aaf..05e1d03 100644 --- a/src/Update.hs +++ b/src/Update.hs @@ -67,13 +67,22 @@ logFileName = do putStrLn ("Using log file: " <> logFile) return logFile +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 - logFile <- logFileName - let log = log' logFile - T.appendFile logFile "\n\n" + 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,7 +136,7 @@ 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 case updated of @@ -161,7 +170,7 @@ updatePackage :: IO (Either Text ()) updatePackage 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 +178,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 @@ -249,9 +254,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 + result <- Nix.resultLink + publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs publishPackage :: MonadIO m => @@ -303,17 +307,16 @@ 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 + when (batchUpdate . options $ updateEnv) + (lift untilOfBorgFree) let base = if numPackageRebuilds opDiff < 100 then "master" else "staging" - lift $ - GH.pr - base - ( prMessage + let prMsg = prMessage updateEnv isBroken metaDescription @@ -329,7 +332,9 @@ publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs = d (outpathReport opDiff) cveRep cachixTestInstructions - ) + if (doPR . options $ updateEnv) + then lift $ GH.pr base prMsg + else liftIO $ T.putStrLn prMsg Git.cleanAndResetTo "master" commitMessage :: UpdateEnv -> Text -> Text diff --git a/src/Utils.hs b/src/Utils.hs index 456a67e..4aaf3ae 100644 --- a/src/Utils.hs +++ b/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 diff --git a/test/RewriteSpec.hs b/test/RewriteSpec.hs index 886a175..a986f57 100644 --- a/test/RewriteSpec.hs +++ b/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 From 06a9f32830195ec44ddf8e0761bc9e263e8ba69b Mon Sep 17 00:00:00 2001 From: Ryan Mulligan Date: Sun, 5 Apr 2020 21:20:08 -0700 Subject: [PATCH 2/3] separate update logic for single-package interactive updating case --- app/Main.hs | 9 ++++--- src/Update.hs | 73 +++++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 68 insertions(+), 14 deletions(-) diff --git a/app/Main.hs b/app/Main.hs index f84ea91..d3e09d8 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -16,7 +16,7 @@ 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) @@ -131,12 +131,15 @@ main = do P.setEnv "PAGER" "" True P.setEnv "GITHUB_TOKEN" (T.unpack token) True updateAll (Options pr True token cachix outpaths) updates - Update UpdateOptions {pr, cachix, outpaths} update -> do + Update UpdateOptions {pr, cachix} update -> do token <- getGithubToken setupNixpkgs token P.setEnv "PAGER" "" True P.setEnv "GITHUB_TOKEN" (T.unpack token) True - updateAll (Options pr False token cachix outpaths) update + 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 diff --git a/src/Update.hs b/src/Update.hs index 05e1d03..d1cb9db 100644 --- a/src/Update.hs +++ b/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, @@ -138,7 +140,7 @@ updateLoop o log (Left e : moreUpdates) mergeBaseOutpathsContext = do updateLoop o log (Right (pName, oldVer, newVer, url) : moreUpdates) mergeBaseOutpathsContext = do 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 @@ -163,12 +165,12 @@ 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 pr = doPR . options $ updateEnv -- @@ -255,7 +257,7 @@ updatePackage log updateEnv mergeBaseOutpathsContext = -- Publish the result lift . log $ "Successfully finished processing" 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 => @@ -265,7 +267,7 @@ publishPackage :: Text -> Text -> Text -> - Set ResultLine -> + Maybe (Set ResultLine) -> [Text] -> ExceptT Text m () publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs = do @@ -312,10 +314,6 @@ publishPackage log updateEnv oldSrcUrl newSrcUrl attrPath result opDiff msgs = d isBroken <- Nix.getIsBroken attrPath when (batchUpdate . options $ updateEnv) (lift untilOfBorgFree) - let base = - if numPackageRebuilds opDiff < 100 - then "master" - else "staging" let prMsg = prMessage updateEnv isBroken @@ -329,11 +327,15 @@ 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 lift $ GH.pr base prMsg + 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" @@ -527,3 +529,52 @@ 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 + 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] + ---------------------------------------------------------------------------- + -- + -- 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 From d320593ccb7732aa0fbd760d8a9afb49ead8e15d Mon Sep 17 00:00:00 2001 From: Ryan Mulligan Date: Sun, 5 Apr 2020 21:27:39 -0700 Subject: [PATCH 3/3] move rewriter calls to Rewrite module --- src/Rewrite.hs | 9 +++++++++ src/Update.hs | 12 ++---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/Rewrite.hs b/src/Rewrite.hs index e625ae2..6bc1e50 100644 --- a/src/Rewrite.hs +++ b/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) diff --git a/src/Update.hs b/src/Update.hs index d1cb9db..0511537 100644 --- a/src/Update.hs +++ b/src/Update.hs @@ -230,11 +230,7 @@ updatePackageBatch 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 @@ -555,11 +551,7 @@ updatePackage o updateInfo = do -- 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