Handle noisy stdout from cabal (e.g. darcs warnings)#499
Handle noisy stdout from cabal (e.g. darcs warnings)#499jmcarthur wants to merge 3 commits intohaskell:masterfrom
Conversation
When tools like darcs prepend warnings to stdout (e.g. "WARNING: creating a nested repository."), versionMaybe in ProgramVersions failed to parse the version string. Apply trim (extract last line, strip trailing whitespace) before parsing, matching what getRuntimeGhcVersion already did in Environment.hs. Move trim to Cradle.Utils as a shared utility and import it in both ProgramVersions and Environment.
cabalGhcDirs and cabalProcess used trimEnd on the output of cabal exec -- ghc --print-libdir and cabal exec -- ghc -e getExecutablePath. trimEnd only strips trailing whitespace, so when cabal pollutes stdout (e.g. darcs "WARNING: creating a nested repository."), the paths get garbage prepended. Use trim (extract last line) instead, consistent with the versionMaybe fix.
callCabalPathForCompilerPath parses JSON from 'cabal path
--output-format=json', but tools like darcs can prepend warning lines
to stdout. Strip characters before the first '{' so JSON parsing
succeeds even with noisy output, avoiding the slow fallback to
cabal exec.
|
Have you seen #485? The issue describes a slightly better way to parse the output without semi-random Perhaps this could be a good idea? |
|
I think you're right. I won't look at it for at least a few days (TBH, this isn't super high priority for me right now), but I'll get around to it. |
|
In fact, I've realized since fixing this bug for myself that, at least for my private projects, I don't think I want to depend on packages via source control, because it causes the build to check upstream every time (I'm not pinning the versions since it's private and I control the dependency anyway), so I'm probably going to just go back to relative, local paths. So this bug doesn't actually matter to me anymore. I'll still take a stab at doing it the better way, maybe within a week, but if it doesn't work out easily, I'll probably abandon it. |
|
Sounds sensible, thank you for raising this issue and providing an idea to fix it! Hope to see you back in a couple of weeks ;) |
2 participants
Fixes #498.
When a cabal project has a
source-repository-packagewithtype: darcs, cabal triggersdarcs cloneduring various operations. darcs likes to printWARNING: creating a nested repository.to stdout, and this noise ends up in the output of several commands that hie-bios runs and parses. The result is that HLS fails to start entirely in darcs-managed projects with darcs dependencies.There were three places that needed fixing:
versionMaybein ProgramVersions.hs was parsing the raw output of--numeric-versionwithout stripping leading noise. Thetrimfunction (take the last line, strip trailing whitespace) already existed in Environment.hs for exactly this kind of problem; the comment even mentions Stack's 7zip noise. I movedtrimtoCradle.Utilsso both modules can use it.cabalGhcDirsandcabalProcessin Cabal.hs were usingtrimEnd(strip trailing whitespace only) on the output ofcabal exec -- ghc --print-libdirandcabal exec -- ghc -e getExecutablePath. This meantHIE_BIOS_GHCandHIE_BIOS_GHC_ARGSgot the darcs warnings prepended to the actual paths. Changed these to usetrimas well.callCabalPathForCompilerPathparses JSON fromcabal path --output-format=json, but the darcs warnings appear before the JSON object. I added adropWhileto skip to the first{before parsing. Without this fix it technically still works (it falls back to the slowercabal execpath) but it does so on every single file load, making startup very slow.Having to apply essentially the same fix in so many places feels like a code smell. There might be an opportunity to centralize the output cleanup closer to the process-running layer so that individual call sites don't each have to remember to strip noise.
I added an integration test for the
versionMaybepath using a bios cradle with a noisy GHC wrapper script. The Cabal.hs fixes are harder to test without darcs in CI, but they use the sametrimfunction. I tested the whole thing end-to-end against a real darcs project and HLS starts up and type-checks everything. Open to suggestions on better test coverage.