Print warnings to stderr

[Benjmhart]

Description of the change

As detailed in #256

This PR renames echo and echoStr to output & outputStr for stdout operations
(if there was a different expectation about the implementation details of output let me know)

It also adds logDebug, logWarning, and logError (and removes echoDebug).

This appeared to break two tests, which have an adjusted implementation to account for both stderr and stdout.

Checklist:

• Added the change to the "Unreleased" section of the changelog
• Added some example of the new feature to the README
• Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

[f-f]

@Benjmhart looks great! 👏

One thing that I feel I didn't clarify enough in #256 (comment) is that I'd like something like logInfo (and I guess we'd need logInfoStr too) to replace most of the current usages of echo - i.e. all "application logging" should go to stderr - so then we'd use echo/output for "application output", in commands like spago sources, spago list-packages, spago run, etc

[Benjmhart]

I'll do a follow up PR with the logInfo fn

[Benjmhart]

hmm, looks like a couple tests that failed in CI may be related to the changes here, I'll try and address later today

[f-f]

@Benjmhart it might be that you missed the update for some of the fixtures. Note that in the failing tests there are two different fixtures being checked, depending on the presence of psa on the path:
https://github.com/spacchetti/spago/blob/336b5edc7cd5b3a03a440b09a0c1a54df063ad6c/test/SpagoSpec.hs#L523-L526

These tests also make use of verbose output so we might need to match both stderr and stdout

[Benjmhart]

hmm these tests fail for different reasons on my local environment, but I'll make adjustments based on the CI results.

@f-f - I'm getting several tests failing with this IOException of type invalidArgument

is there some missing environmental dependency for tests that you can think of that should be documented, perhaps in the CONTRIBUTING.md file?

[Benjmhart]

@f-f not quite sure what the root of this is, all these control characters weren't present in the original text file. maybe this was being emitted to stderr before and it wasn't being tested?

for the moment I'll just add a function to either clear or unescape the ANSI codes so it can be sanely compared to the fixture file

[Benjmhart]

hm, at this point i'm going to stop until I can get the test suite running correctly. let me know if you have any idea what the cause of the original invalid byte sequence error might be

[Benjmhart]

ok, looks like I've resolved the issues in pipeline. sorry for that.

I've documented the issue under the 'running tests' section of CONTRIBUTING.md so others can work around it.

[f-f]

@Benjmhart sorry for the trouble! The escape sequences should be there because psa uses color in the output. So I'd say let's not have the stripAnsi machinery, as we can just match on the output

..if the encoding is correct, which is what you figured afterwards.

However, rather than asking users/devs to set envvars to run tests, let's just set UTF8 encoding by default. The way we do it in other places is by calling GHC.IO.Encoding.setLocaleEncoding GHC.IO.Encoding.utf8

I think for setting this up with our test machinery is the following (documented here):

• the test/Spec.hs file should contain {-# OPTIONS_GHC -F -pgmF hspec-discover -optF --module-name=Spec #-}
• the test/Main.hs should contain the following:

  module Main where
  
  import Test.Hspec.Runner
  import qualified Spec
  import qualified GHC.IO.Encoding
  
  main :: IO ()
  main = do
    GHC.IO.Encoding.setLocaleEncoding GHC.IO.Encoding.utf8
    hspecWith defaultConfig Spec.spec

Could you try to see if this works?

[Benjmhart]

@f-f unfortunately stack will not compile the test suites using that technique, at least on my system, I'm getting an error:

...
Building test suite 'spec' for spago-0.10.0.0..
illegally!  
[6 of 6] Compiling Spec
            
<no location info>: error:
    output was redirected with -o, but no output will be generated
because there is no Main module.

I assume this is a GHC error resulting from the options in the HSpec docs. if you're not sure how to resolve I may open an issue with Hspec, as I don't see any documentation on this issue

This was with the /test/Spec.hs file and /test/Main.hs file as you described

anyhow, I've resolved conflicts, run a test build, and removed the stripAnsi machinery. the test passes locally.

let me know if these needs further adjustment, or if you'd like me to pursue the issue on HSpec, I'm happy to put in more work to get this where it needs to be.

[f-f]

@Benjmhart right, I forgot to mention that you need to add main: Main to the test suite in the package.yaml to make this work. Anyways don't worry about this, let's merge this PR now and we can take care of the rest in subsequent PRs

Thank you for the great work! 💯

[Benjmhart]

@f-f facepalm I knew it was something obvious. thanks for your help & patience.

I'll start work on a PR for application output vs application logging over the next few days.

[f-f]

@Benjmhart you're welcome 🙂