-
-
Notifications
You must be signed in to change notification settings - Fork 195
travis: test both Stack and non-stack (Cabal) #174
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
Conversation
No longer WIP, let's take a look. So we'll have the hermetic builds and dependency checking provided by Stack, but do not leave 7.6 behind. One big question is whether the extra complexity is maintainable, which we should discuss. It is possible that we can have 7.6 be the only version that uses cabal-install (Note that the current iteration of this PR tests 7.6, 7.8, 7.10, 8.0 with cabal-install, which may potentially be gratuitous.) |
configlet . | ||
case "$BUILD" in | ||
stack) | ||
set -e |
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.
If configlet
fails, why continue testing?
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 take it this means we should move set -e
above configlet .
I agree.
updated to move $ git diff e4b68e6 14f272b
diff --git a/.travis.yml b/.travis.yml
index c9fb04f..759ba58 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -64,10 +64,10 @@ install:
script:
- |
+ set -e
configlet .
case "$BUILD" in
stack)
- set -e
for exercise in ${TRAVIS_BUILD_DIR}/exercises/* ; do
pushd ${exercise}
# `stack --word-dir` fails if not targeting a subdirectory, so we just |
Moved some things to separate lines if possible. $ git diff faf9502 247f0cf
diff --git a/.travis.yml b/.travis.yml
index 1b596f4..5b8a467 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -40,8 +40,8 @@ matrix:
fast_finish: true # is released, so we list them here.
before_install:
+ - export PATH="${TRAVIS_BUILD_DIR}/bin:$PATH" # For {,fetch-}configlet.
- |
- export PATH="${TRAVIS_BUILD_DIR}/bin:$PATH" # For {,fetch-}configlet.
case "$BUILD" in
stack)
mkdir -p ${HOME}/bin # Create folder for stack.
@@ -55,18 +55,18 @@ before_install:
esac
install:
+ - travis_retry fetch-configlet
- |
set -e
- travis_retry fetch-configlet
if [ "$BUILD" = "stack" ]; then
travis_retry curl -L https://www.stackage.org/stack/linux-x86_64 -o pack.tgz
tar xzf pack.tgz --wildcards --strip-components=1 -C ${HOME}/bin '*/stack'
fi
script:
+ - configlet .
- |
set -e
- configlet .
case "$BUILD" in
stack)
for exercise in ${TRAVIS_BUILD_DIR}/exercises/* ; do |
- configlet . | ||
- | | ||
set -e | ||
case "$BUILD" in |
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.
In script:
Travis has a special behavior. If a command fails, Travis will fail everything, but it will not shortcut evaluation, so every entry will be executed.
I used set -e
and only one command in #162 to avoid that.
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.
Ah interesting. Would we like this to happen? Consider this case:
If I'm adding a new exercise and have forgotten to add it to config.json AND its tests are failing: Maybe I want to see both of those things. If we stop the build immediately on configlet
failing, then I have to fix that first, and only after I do that will I find out my tests are failing too. Is that OK?
Then again, the argument may be "well maybe I should have run the tests locally to see whether they were passing" which... is valid. Maybe the tests only fail on an old version of GHC though?
I'm OK with either of these behaviors, but we should make the explicit decision.
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 prefer to fail ASAP:
- Most of the time we are waiting for Travis, so faster is better.
- Usually the changes are small, so we have just one error.
- Travis is just a final check, it should be tested locally.
- It saves resources for other projects.
- It marginally decreases global warming. 😬
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.
OK, in that case I'll move configlet back into the single command, after set -e
OK, I pretty much squashed #162 and this together, this avoids the delete and then re-add of bootstrap.sh. The only noticeable change is I also tried to add explanatory comments. The functionality as Travis sees it should not change. |
The work in #162 is advantageous because we can check our dependencies and have hermetic builds (do not break on upstream changes). One question about whether to merge it was the necessity to drop GHC 7.6. This commit introduces to Travis the ability to test with either Stack or without (the old way of doing things). We test 7.6, 7.8, 7.10, 8.0 with cabal-install as we did before, and also Stack as done in #162. Closes #162 (is based on it) Closes #124 (explanatory comments provided) Closes #153 (based on #162 which also closed #153)
This removes the need to set PATH in bootstrap.sh
Rebased now that #179 is in. I'll assume we want to keep the It's worth asking because, well, if the only reason we want to keep the |
About keeping the other GHC versions with cabal, I don't see the need, but that doesn't bother me at all. |
The work in #162 is advantageous because we can check our dependencies
and have hermetic builds (do not break on upstream changes).
One question about whether to merge it was the necessity to drop GHC
7.6.
This commit introduces to Travis the ability to test with either Stack
or without (the old way of doing things). We test 7.6, 7.8, 7.10, 8.0
with cabal-install as we did before, and also Stack as done in #162.
Closes #162 (is based on it)
Closes #124 (explanatory comments provided)
Closes #153 (based on #162 which also closed #153)