gregwebs/Shelly.hs

upgrade to 1.9 breaks our test suite

Closed this issue · 10 comments

bfrk commented

I wanted to use 1.9 so we can support ghc >= 8.8. But this breaks out test suite. It no longer finds any of our shell tests.

I looked at the changelog for 1.9 and see no mention of breaking changes, in fact there are no entries for 1.9 at all. I can understand that sometimes one has to break compatibility, but not even communicating what was changed (or why) is really quite unfriendly to users. I'll now have to debug our code and try to reverse engineer why the shell tests aren't found and find a fix for this.

I am seriously considering to re-write our code to no longer depend on shelly. This has not been the first such incident, see #176 and #177, which were incompatibilities on Windows; this one breaks even on Linux.

andreasabel commented

In 1.9, shelly switched from system-filepath to filepath which changed the type of FilePath, a breaking change.

I am recovering the changelog for 1.9 and will release it with 1.10.

adinapoli commented

@andreasabel Would you accept patches to retrofit the old behaviour into Shelly >= 1.9 ? To simplify to the extreme what @bfrk was reporting, given a simple run_me.sh in the $PWD and a TeshShelly.hs like such:

{-# LANGUAGE OverloadedStrings #-}
module TestShelly where

import Shelly

main :: IO ()
main = shelly $ verbosely $
  run_ "./run_me.sh" []

This simple Haskell script will behave differently depending on the Shelly version:

stack --resolver lts-17.15 --no-system-ghc exec --package shelly-1.9.0 runghc -- TestShelly.hs
./run_me.sh
TestShelly.hs: 
Ran commands: 
echo './run_me.sh'
./run_me.sh
which ./run_me.sh

Exception: user error (shelly did not find ./run_me.sh in the PATH

vs.

stack --resolver lts-14.15 --no-system-ghc exec --package shelly-1.8.0 runghc -- TestShelly.hs
./run_me.sh
Hello!

The culprit might easily be the switch to filepath, as you mention, but I think this is problematic for many reasons:

  1. Most of us were forced to upgrade to >= 1.9 if we wanted to build with recent GHCs, due to the MonadFail changes. In this sense we had no option; there was no intermediate 1.8.x release which would just remove the fail calls in Shelly;

  2. Most of us have lots of Shelly code in the codebase which runs scripts like in my example, and now they will blow up at runtime;

  3. The changelog doesn't mention any way to restore the old behaviour, it just mentions that a breaking change has been made, but it doesn't say what affects.

I feel like if we were to either restore the old behaviour or at least have a migration path somewhere in the README, this would help a lot, already.

Thanks! 😉

andreasabel commented

@andreasabel Would you accept patches to retrofit the old behaviour into Shelly >= 1.9 ? To simplify to the extreme what @bfrk was reporting, given a simple run_me.sh in the $PWD and a TeshShelly.hs like such:

{-# LANGUAGE OverloadedStrings #-}
module TestShelly where

import Shelly

main :: IO ()
main = shelly $ verbosely $
  run_ "./run_me.sh" []

This simple Haskell script will behave differently depending on the Shelly version:

stack --resolver lts-17.15 --no-system-ghc exec --package shelly-1.9.0 runghc -- TestShelly.hs
./run_me.sh
TestShelly.hs: 
Ran commands: 
echo './run_me.sh'
./run_me.sh
which ./run_me.sh

Exception: user error (shelly did not find ./run_me.sh in the PATH

This looks wrong, and, as you say, it is a regression introduced in 1.9.0. This should be fixed, of course. And we should have a regression test for this.

The culprit might easily be the switch to filepath, as you mention, but I think this is problematic for many reasons:

1. Most of us were forced to upgrade to `>= 1.9` if we wanted to build with recent GHCs, due to the `MonadFail` changes. In this sense we had no option; there was no intermediate 1.8.x release which would just remove the `fail` calls in `Shelly`;

I agree that such an intermediate release would have the better migration path.

Do you advocate a release of such a 1.8.2? This could help for a while, but maybe not in the long run. It would maybe encourage users to stay on 1.8.x, and fixes would have to be back-ported from >=1.10 to 1.8.x to entertain these users.

2. Most of us have lots of Shelly code in the codebase which runs scripts like in my example, and now they will blow up at runtime;

3. The changelog doesn't mention any way to restore the old behaviour, it just mentions that a breaking change has been made, but it doesn't say _what_ affects.

This is bad, of course. I think the maintainers of shelly were a bit burned-out at that time; 1.9 was a somewhat unorderly release.
I only stepped up as a steward because the package badly needed help.

I feel like if we were to either restore the old behaviour or at least have a migration path somewhere in the README, this would help a lot, already.

Help with that is most welcome!

adinapoli commented

Thanks @andreasabel for your prompt reply and no need to apologise -- I most definitely have been on the other side of the "barricade" and I understand that maintaining OSS (most of the time outside the realm of the normal working hours) can be quite taxing at times 😉

Do you advocate a release of such a 1.8.2?

For our production systems, a 1.8.2 release would be a boon while we wait for the bug to be fixed -- if it doesn't cost you folks too much effort, it would be highly appreciated 😉 I take your point that users might not be encouraged to switch to >= 1.9 this way, but at least they have a choice, and they can jump the latest and greatest train when they feel up to 😉

  • 👍1
andreasabel commented

For our production systems, a 1.8.2 release would be a boon while we wait for the bug to be fixed

Ok, hang on, I get a go at it...

  • ❤️1
andreasabel commented

Er, that looked easy, but it was a full working day after all...
@adinapoli A release candidate is ready at #215 now. Please test it in your projects to check whether it does what you want. Testing instructions are at #215.

adinapoli commented

Hey @andreasabel , thank you so much for doing the extra work to cut a 1.8.2 release!

I am happy to report that it did the trick:

stack repl -v
2022-03-25 08:20:20.194183: [debug] Run process within ...
 -package-id=base-4.14.1.0 -package-id=shelly-1.8.2-EfEe5Lirro5FoPPmhr1xhW -i...
GHCi, version 8.10.4: https://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /Users/adinapoli/dotfiles/ghci/.ghc/ghci.conf
[1 of 2] Compiling Main             ( /Users/adinapoli/programming/haskell/playground/app/Main.hs, interpreted )
[2 of 2] Compiling TestShelly       ( TestShelly.hs, interpreted )
Ok, two modules loaded.
Loaded GHCi configuration from /private/var/folders/tj/t3094j214bq_qvgmc42v9grr0000gn/T/haskell-stack-ghci/f2aa98e2/ghci-script
> import qualified TestShelly as TS
> TS.main
./run_me.sh
Hello!
>

Note the -package-id=shelly-1.8.2-EfEe5Lirro5FoPPmhr1xhW there.

Thank you so much!

  • 🎉1
andreasabel commented

@adinapoli : Great, thanks for the feedback:

I published: https://hackage.haskell.org/package/shelly-1.8.2

adinapoli commented

@adinapoli : Great, thanks for the feedback:

I published: https://hackage.haskell.org/package/shelly-1.8.2

Super, thanks again for your help!

As regards why the switch to filepath is causing Shelly to regress, that's a battle for another day. I might myself take a look at some point, and issue a PR if I managed to found anything (unless you or somebody else beat me to the punch) 😉

andreasabel commented

I might myself take a look at some point, and issue a PR if I managed to found anything

Yes, please do so.
I am afraid I have limited own interest in this bug so far, as my product BNFC hasn't run into such problems. So it would be great if the community helped with that!

  • 👍1