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.

Sign up for GitHub

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

ci/cron: move concurrency from Bash to Haskell #7696

Merged
merged 1 commit into from
Oct 15, 2020
Merged

ci/cron: move concurrency from Bash to Haskell #7696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ci/cron/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ da_haskell_binary(
srcs = glob(["src/**/*.hs"]),
hackage_deps = [
"aeson",
"async",
"base",
"case-insensitive",
"containers",
Expand Down
23 changes: 8 additions & 15 deletions ci/cron/src/Main.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Data.Function ((&))
import Data.Semigroup ((<>))
import System.FilePath.Posix ((</>))

import qualified Control.Concurrent.Async
import qualified Control.Exception
import qualified Control.Monad as Control
import qualified Control.Monad.Extra
Expand Down Expand Up @@ -269,21 +270,13 @@ docs = do

download_assets :: FilePath -> GitHubRelease -> IO ()
download_assets tmp release = do
shell_ $ unlines ["bash -c '",
"set -euo pipefail",
"eval \"$(dev-env/bin/dade assist)\"",
"cd \"" <> tmp <> "\"",
"PIDS=\"\"",
"for ass in " <> unwords (map (show . uri) $ assets release) <> "; do",
"{",
"wget --quiet \"$ass\" &",
"}",
"PIDS=\"$PIDS $!\"",
"done",
"for pid in $PIDS; do",
"wait $pid",
"done",
"'"]
Control.Concurrent.Async.forConcurrently_ (map uri $ assets release) (\url -> do
Copy link
Contributor

Choose a reason for hiding this comment

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

forConcurrently_ will spawn an unbounded number of threads. I guess we don’t care since the number of assets is small? If we do care, the easiest solution is usually just to throw a semaphore around the map function you pass to forConcurrently_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Bash code this replaces was also unbounded, so at least this is not getting worse. 🙂

I'm not concerned; we're not going to have a very big number of files anyway. If you are concerned, I can add a semaphore with, say, 30 slots, so it won't change anything for the happy path (16 assets atm) but will shield against future issues. I'd prefer to do that in a future PR, though.

shell_ $ unlines ["bash -c '",
"set -euo pipefail",
"eval \"$(dev-env/bin/dade assist)\"",
"cd \"" <> tmp <> "\"",
"wget --quiet \"" <> show url <> "\"",
Comment on lines +275 to +278
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we could just call wget directly and -o to avoid the cd but I’ll leave that up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ì'm planning to remove wget in my next PR (perhaps next-next now, if the next one is the semaphore), so I don't see much point in trying to make the Bash code nicer. I would point though that we are not in the tmpdir in terms of Haskell process cwd as far as I can tell, so the cd there is not superfluous as it stands.

"'"])

verify_signatures :: FilePath -> FilePath -> String -> IO String
verify_signatures bash_lib tmp version_tag = do
Expand Down