-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add nightly scheduled build of saw-script #889
Conversation
ece8ee3
to
92cb694
Compare
490e6f5
to
ace4c26
Compare
fa9717c
to
1da58a7
Compare
1da58a7
to
72c7c75
Compare
@@ -158,7 +162,7 @@ install_system_deps() { | |||
install_yasm & | |||
wait | |||
export PATH=$PWD/$BIN:$PATH | |||
echo "$PWD/$BIN" >> $GITHUB_PATH | |||
echo "$PWD/$BIN" >> "$GITHUB_PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you quote everything like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrational levels of paranoia combined with following all the warnings shellcheck provides result in lots of quoting. It may technically be unnecessary in this case, but there's no reason not to; unless minimizing diffs is a valid concern, I suppose
@@ -74,16 +77,17 @@ jobs: | |||
path: | | |||
${{ steps.setup-haskell.outputs.cabal-store }} | |||
dist-newstyle | |||
key: cabal-${{ runner.os }}-${{ matrix.ghc }}-${{ hashFiles('**/cabal.GHC-*') }}-${{ github.sha }} | |||
key: cabal-${{ runner.os }}-${{ matrix.ghc }}-${{ hashFiles(format('cabal.GHC-{0}.config', matrix.ghc)) }}-${{ github.sha }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicked this trick from rust's github actions CI script :) it's pretty great.
@@ -119,8 +123,7 @@ jobs: | |||
env.RELEASE && matrix.ghc == '8.8.4' | |||
run: | | |||
.github/ci.sh build_cryptol | |||
exe="$(cd deps/cryptol && cabal exec which cryptol)" | |||
cp "$exe" "dist/bin" | |||
.github/ci.sh extract_exe "cryptol" "dist/bin" "deps/cryptol/dist-newstyle" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this code could be generally refactored out completely... cabal install to a prefix directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cabal install, unfortunately, does not always do the right thing. It needs to be cabal build
and then running find in dist-newstyle. But here since we're building cryptol in a subdir, I have to do the weird thing there.
Ideally, saw-script relies on the latest hackage version of cryptol and the binary is downloaded from wherever we end up storing prebuilt binaries (probably just github releases for now). Then we won't need to do any of this and instead we can just download the appropriate binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(that said. "cabal build + find in dist-newstyle + copy out to specified directory" is a good candidate for a helper function somewhere)
@@ -137,7 +140,7 @@ jobs: | |||
path: "dist/bin/saw" | |||
|
|||
- shell: bash | |||
if: "steps.test.outcome == 'failure'" | |||
if: "steps.test.outcome == 'failure' || steps.cabal-test.outcome == 'failure'" | |||
name: Warn if tests failed | |||
run: echo "::error ::Test Suite Failed. Pipeline allowed to pass until tests are reliable." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to handle a situation like this is regression testing (indicate failure when tests that were previously passing are now failing), maybe it would be worthwhile to implement something like this that can be used generally for cabal tests?
@@ -0,0 +1,100 @@ | |||
name: Nightly Builds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates a lot of logic, wondering if nightly can't be implemented in build.yml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, I had things separated out in different workflows before I realized that github had almost zero support for coordinating things between workflows. As such, were I to redo it today, I'd have a single workflow file to not duplicate the logic. Unfortunately, that can end up with a lot of complex logic around when to skip/run jobs if we're not careful.
But yeah, it makes sense to eventually roll this into build.yml (and likely rename it to ci.yml).
No description provided.