Symlink traversing logic in build scripts

[snowleopard]

Hadrian build scripts build.sh, build.cabal.sh etc contain mysterious (to me) symlink-traversing logic, which may be confusing in certain setups, as reported in https://ghc.haskell.org/trac/ghc/ticket/15576.

@hvr I think this logic was introduced by you. I couldn't recall why. Could you please clarify? Do we still need it? If yes, how do we avoid the confusion as described in the above ticket?

I'm tempted to classify this as a bug, since it leads to producing build results in an unexpected place.

[alpmestan]

@hvr on IRC:

<hvr> I'm trying to rememebr the rationale, but it's likely that we need to know the normalised pathname of where the script lives
<hvr> as cabal needs to be able to locate the .cabal files and all that
<hvr> and passing relative pathnames to cabal is often not a good idea
<hvr> so, if we can ditch support for cabal < 2.2
<hvr> stuff gets more uniform
<hvr> as then we only have to figure out how to support the
<hvr> "$CABAL" new-run hadrian --  --directory "$absoluteRoot/.." "$@"
<hvr> invocation

[hvr]

To elaborate a bit; the important bit is that we want build.cabal.sh to do the right thing independently on what $PWD currently points to (after all, $PWD is supposed to give the build-system a hint which implicit build-target I want it to build right now depending on which folder I'm in); in order to do that, we need

1. to make sure that cabal new-{run,build} are able to find their (cabal) project root (NB: cabal new-run intentionally does not change the CWD before invoking the executable)
2. make sure hadrian is able to locate the GHC project root

It's also worth noting that the script could effectively be reduced to a single invocation of new-run (assuming we have a trivial cabal.project file to help cabal out):

"$CABAL" --project-file="$absoluteRoot/cabal.project"  new-run hadrian -- --directory "$absoluteRoot/.." "$@"   

No need to cd anywhere anymore, assuming hadrian can (like cabal) handle it if it's told where the project root is.

[snowleopard]

we want build.cabal.sh to do the right thing independently on what $PWD currently points to

@hvr Aha, I see. This makes sense, although Hadrian currently doesn't support $PWD-based build targets.

It's also worth noting that the script could effectively be reduced to a single invocation of new-run

Let's do this! I'm happy to sacrifice the support of older versions of Cabal if need be.

Could you send a PR?

[hvr]

@snowleopard what about locating the project-root? do we need to change how that is accomplished?

[snowleopard]

@hvr I am tempted to first support only the most basic version of the build script, which assumes that it is run from Hadrian folder, i.e. hadrian/build.sh ....

Later we can think how we can implement more a feature-rich script, which could simply be calling this most basic script with the right parameters.

[alpmestan]

which assumes that it is run from Hadrian folder, i.e. hadrian/build.sh ....

you mean "which assumes that it is run from the top of the GHC source tree" ?

[snowleopard]

Ah, yes, that's right.

[hvr]

that'd be quite a regression compared to make which would make me curse quite a lot... :-(

...and actually I've got an idea how to salvage this w/o resorting to readlink

[snowleopard]

Yes, but let's do one step at a time please :)

• Let's have a basic build script living in hadrian/build.sh, with zero magic.
• Then I'm happy to introduce more sophisticated versions, perhaps living at the top of the GHC tree that can have all sorts of PWD-detection and rely on some built-in Hadrian support.

Right now we have neither!

[hvr]

@snowleopard fair enough; I've tweaked the script a bit, how does this look?

#!/usr/bin/env bash

CABAL=cabal
CABFLAGS="--disable-documentation --disable-profiling -v0"
PROJ="$PWD/hadrian/cabal.project"

set -euo pipefail

if ! [ -f "$PROJ" ]; then
    echo "Current working directory must be GHC's top-level folder"
    exit 2
fi

if ! type "$CABAL" > /dev/null; then
    echo "Please make sure 'cabal' is in your PATH"
    exit 2
fi

CABVERSTR=$("$CABAL" --numeric-version)
CABVER=( ${CABVERSTR//./ } )

if [ "${CABVER[0]}" -gt 2 -o "${CABVER[0]}" -eq 2 -a "${CABVER[1]}" -ge 2 ]; then
    # New enough Cabal version detected, so let's use the superior new-build + new-run
    # modes. Note that pre-2.2 Cabal does not support passing additional parameters
    # to the executable (hadrian) after the separator '--', see #438.

    "$CABAL" --project-file="$PROJ" new-build $CABFLAGS -j exe:hadrian
    "$CABAL" --project-file="$PROJ" new-run   $CABFLAGS    exe:hadrian -- \
        --lint \
        --directory "$PWD" \
        "$@"

else
    echo "cabal version too old; you need at least cabal-install 2.2"
    exit 2
fi

There's a few optimization one could consider; like e.g. compiling the local lib:Cabal w/ -O0 (but you'd have to benchmark whether it's tolerable), or possibly injecting ghc-options: -j into lib:Cabal

[hvr]

@snowleopard oh and btw, do we need a powershell script for windows? or does windows use bash as well?

[snowleopard]

@hvr Thanks! Why do we need $PWD? If this script only supports invocation from GHC root we can just replace it with ., can't we?

There's a few optimization one could consider

Indeed, I'll give it a try.

oh and btw, do we need a powershell script for windows? or does windows use bash as well?

We do have build.stack.bat and build.bat that use Stack to download MSYS and can be executed from Windows command line, see https://github.com/snowleopard/hadrian/blob/master/doc/windows.md. This is how our AppVeyor CI works.

[snowleopard]

By the way, we've got quite a lot of scripts, perhaps we should move all but build.sh and build.bat into a subdirectory, say scripts, to have a simpler top-level directory.

[alpmestan]

Yes, sounds like a good idea to me.

[hvr]

Thanks! Why do we need $PWD? If this script only supports invocation from GHC root we can just replace it with .., can't we?

Are you sure you really meant to say ..? That would put you outside the GHC source-tree.

Also, it's more robust currently to pass cabal an absolute path (I'm suspecting a bug in some cabal versions you may otherwise trigger), and I'd rather play it safe. Is there any problem with being more explicit?

We do have build.stack.bat and build.bat that use Stack to download MSYS and can be executed from Windows command line, see https://github.com/snowleopard/hadrian/blob/master/doc/windows.md.

Ok, so we need a build.cabal.bat then? I'd really prefer to offer a uniform cabal based workflow on all platforms for various reasons.

[hvr]

By the way, we've got quite a lot of scripts, perhaps we should move all but build.sh and build.bat into a subdirectory, say scripts, to have a simpler top-level directory.

Btw, what's the point of the hadrian/build.sh script? Why having a 35-line bash script to do a simple indirection? couldn't that merely be a symlink to the build.cabal.sh script at this point?

[snowleopard]

Are you sure you really meant to say ..?

@hvr Apologies, I later edited it to . :)

Also, it's more robust currently to pass cabal an absolute path (I'm suspecting a bug in some cabal versions you may otherwise trigger), and I'd rather play it safe. Is there any problem with being more explicit?

OK, I'll try your current version then. My only concern is unnecessary complexity. If it is necessary, I'll add a comment about the potential Cabal bug.

Ok, so we need a build.cabal.bat then? I'd really prefer to offer a uniform cabal based workflow on all platforms for various reasons.

Yes, we do. I fully agree about the uniform workflow -- I actually tried to make build.cabal.bat work for me on Windows some time ago, but unsuccessfully -- that's why we only have build.stack.bat.

[snowleopard]

Btw, what's the point of the hadrian/build.sh script? Why having a 35-line bash script to do a simple indirection?

I have no idea why it is that long. My preference would be to simply call the default script: the Cabal or Stack based one.

couldn't that merely be a symlink to the build.cabal.sh script at this point?

I prefer to use plain files if symlinks are avoidable. To me, there is always some additional mental overhead associated with symlinks. (Maybe I'm just wrong.)

[snowleopard]

OK, I put together a PR: #668. Please have a look.

@hvr Regarding build.cabal.bat on Windows. Here is what I get when I try to run the same commands as in the build.sh script:

$ cabal --version
cabal-install version 2.2.0.0
compiled using version 2.2.0.1 of the Cabal library

$ cabal --project-file=$PWD/hadrian/cabal.project new-build --disable-documentation --disable-profiling -v0 -j exe:hadrian
cabal.exe: Could not resolve dependencies:
[__0] trying: Cabal-2.4.0.0 (user goal)
[__1] next goal: text (dependency of Cabal)
[__1] rejecting: text-1.2.2.2, text-1.2.2.1, text-1.2.2.0, text-1.2.1.3,
text-1.2.1.2, text-1.2.1.1, text-1.2.1.0, text-1.2.0.6, text-1.2.0.5,
text-1.2.0.4, text-1.2.0.3, text-1.2.0.2, text-1.2.0.0, text-1.1.1.4,
text-1.1.1.3, text-1.1.1.2, text-1.1.1.1, text-1.1.1.0, text-1.1.0.1,
text-1.1.0.0, text-1.0.0.1, text-1.0.0.0, text-0.11.3.1, text-0.11.3.0,
text-0.11.2.3, text-0.11.2.2, text-0.11.2.1, text-0.11.2.0, text-0.11.1.13,
text-0.11.1.12, text-0.11.1.11, text-0.11.1.10, text-0.11.1.9, text-0.11.1.8,
text-0.11.1.7, text-0.11.1.6, text-0.11.1.5, text-0.11.1.3, text-0.11.1.2,
text-0.11.1.1, text-0.11.1.0, text-0.11.0.8, text-0.11.0.7, text-0.11.0.6,
text-0.11.0.5, text-0.11.0.4, text-0.11.0.3, text-0.11.0.2, text-0.11.0.1,
text-0.11.0.0, text-0.10.0.2, text-0.10.0.1, text-0.10.0.0, text-0.9.1.0,
text-0.9.0.1, text-0.9.0.0, text-0.8.1.0, text-0.8.0.0, text-0.7.2.1,
text-0.7.1.0, text-0.7.0.1, text-0.7, text-0.6, text-0.5, text-0.4, text-0.3,
text-0.2, text-0.1 (conflict: Cabal => text>=1.2.3.0 && <1.3)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: Cabal, text

Any idea why this works on Linux and OSX but not Windows?

[hvr]

@snowleopard this looks like your package db doesn't contain text-1.2.3.0

do you happen to have an old ~/.cabal/config file? alternatively you could be suffering from the index-reset hackage-security bug we had over new years eve

So if cabal update doesn't resolve this, please reset your local cabal application data (it's somewhere in %APPDATA% iirc)

PS: For windows, you may need (in case you run into weird permission denied errors) to use --store-dir=C:\SR (as explained e.g. in https://hub.zhox.com/posts/chocolatey-introduction/) to workaround the filesystem PATHMAX

[snowleopard]

@hvr Somehow I managed to make it work, thank you for your pointers! I will add build.cabal.bat too.

[Mistuke]

btw, in case it helps, this is a 1-1 convertion of the sh file

@echo off
set CABAL=cabal
set CABFLAGS="--disable-documentation --disable-profiling -v0"
set PROJ="%CD%/hadrian/cabal.project"

if not exist %PROJ% (
    echo Current working directory must be GHC's top-level folder
    exit /B 2
)

"%CABAL%" 2> NUL
if not %ERRORLEVEL% equ 1 (
    echo Please make sure 'cabal' is in your PATH
    exit /B 2
)

for /F "tokens=*" %%a in ('%CABAL% --numeric-version') do set CABVERSTR=%%a
for /F "delims=. tokens=1,2,3,4" %%a in ("%CABVERSTR%") do (
    set CABMAJOR=%%a
    set CABMINOR=%%b
    set CABREV=%%c
    set CABPATCH=%%d
)

set "_cabal_ok=0"
if %CABMAJOR% gtr 2 set _cabal_ok=1
if %CABMAJOR% equ 1 (
    if %CABMINOR% geq 2 set _cabal_ok=1
)
if %_cabal_ok% equ 1 (
    rem New enough Cabal version detected, so let's use the superior new-build + new-run
    rem modes. Note that pre-2.2 Cabal does not support passing additional parameters
    rem to the executable (hadrian) after the separator '--', see #438.

    "%CABAL%" --project-file=%PROJ% new-build %CABFLAGS% -j exe:hadrian
    "%CABAL%" --project-file=%PROJ% new-run   %CABFLAGS%    exe:hadrian -- ^
        --lint ^
        --directory "%CD%" ^
        "%*"

) else (
    echo cabal version too old; you need at least cabal-install 2.2
    exit /B 2
)

[snowleopard]

Thanks @Mistuke! I've added it, albeit with some tweaks: 710e84d. Had to drop quotes around argument lists CABFLAGS and %*, plus fixed Cabal version check.

[snowleopard]

Closed by #668.

[snowleopard]

Note: there is a separate issue about creating the subdirectory scripts: #475.