Allow arbitrary Cabal tools #987
Conversation
mboes
force-pushed
the
more-cabal-tools
branch
4 times, most recently
from
cd2442d to
e1c2e2c
Compare
haskell/private/cabal_wrapper.sh.tpl
Outdated
| do | ||
| if [[ -n "$entry" ]] | ||
| then | ||
| new_path="$new_path${new_path:+:}$(realpath -m "$entry")" |
There was a problem hiding this comment.
As far as I can tell realpath doesn't exist on macos, so this will probably fail on non nixpkgs macos. This can also be done in bash, similar to here
There was a problem hiding this comment.
macOS+Nix shouldn't be a problem, but bare macOS, yes. A pure bash solution is better. I found this on the Internet:
realpath() {
[[ $1 = /* ]] && echo "$1" || echo "$PWD/${1#./}"
}
which I understand better than the code you link to.
There was a problem hiding this comment.
The linked code does essentially the same thing, with the difference that it keeps relative paths relative. In that case it matters more, as the outputs of ./configure are outputs of the configure step and inputs to later build steps, so absolute paths would certainly leak into cache keys. Here it should matter less, as PATH should typically not end up in the built library/binary.
|
@aherrmann PTAL |
| # Remove any relative entries, because we'll be changing CWD shortly. | ||
| LD_LIBRARY_PATH=$(canonicalize_path $LD_LIBRARY_PATH) | ||
| LIBRARY_PATH=$(canonicalize_path $LIBRARY_PATH) | ||
| PATH=$(canonicalize_path $PATH) |
There was a problem hiding this comment.
I'm a bit surprised that we have to do the absolute name resolution in Bash. I would have thought Bazel would give us tools to do so.
There was a problem hiding this comment.
Any Bazel supplied absolute paths would wreak havoc on the action cache. Nix gets away with this because the store is always in the same location. But the Bazel cache and the action execution sandbox could be anywhere.
There was a problem hiding this comment.
Oh! I get it! It could be quite terrible for shared cache too.
That being said, these global names should not be a dependency nor an output of the target, so I'm not sure it would matter for this particular case.
There was a problem hiding this comment.
Right, I meant for any shared cache. In general there will be paths to inputs going in one or more of the above 3 environment variables.
Cabal has a whitelist of tools, for which we can supply a path explicitly using `--with-PROG`, where `PROG` is the name of the tool. But in practice `Setup.hs` scripts and the build process in general sometimes use other tools that are not part of the whitelist. For example, `postgresql-libpq` calls `pg_config` during configuration. Unlike the whitelist, these tools can only be supplied by ensuring they are reachable from the `PATH`. So we extend the `PATH` to make this true, hoping that not too much else is exposed as a side effect. We could avoid undeclared inputs leaking, but 1. we can't do this without a hefty performance cost on Windows (no symlinks), 2. in practice this should be the responsibility of the workspace rules for each dependency to expose just what we want, 3. Cabal rules are not hermetic to begin with, because they need all manner of standard utilities. Adding a test for this would be awkward, and the feature is at a low risk of regression, so I didn't write one. Testing would require making all of `@stackage`, and hence most tests, depend on Nixpkgs. We don't want that, because of bindist builds.
Cabal has a whitelist of tools, for which we can supply a path
explicitly using
--with-PROG, wherePROGis the name of the tool.But in practice
Setup.hsscripts and the build process in generalsometimes use other tools that are not part of the whitelist. For
example,
postgresql-libpqcallspg_configduring configuration.Unlike the whitelist, these tools can only be supplied by ensuring
they are reachable from the
PATH. So we extend thePATHto makethis true, hoping that not too much else is exposed as a side effect.
We could avoid undeclared inputs leaking, but
symlinks),
rules for each dependency to expose just what we want,
manner of standard utilities.
Adding a test for this would be awkward, and the feature is at a low
risk of regression (since it's already partially tested), so I didn't write one. Testing would require
making all of
@stackage, and hence most tests, depend on Nixpkgs. Wedon't want that, because of bindist builds.