Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new-run should cd to the package directory before running the exe #5001

Closed
fgaz opened this issue Jan 6, 2018 · 9 comments
Closed

new-run should cd to the package directory before running the exe #5001

fgaz opened this issue Jan 6, 2018 · 9 comments

Comments

@fgaz
Copy link
Member

fgaz commented Jan 6, 2018

Most exes(tests,benchs) expect to be run in the package root, but with the introduction of projects this is no longer always true.

This became evident when trying new-run with the cabal-install tests (using #4861) while being in the project root.

Relevant lines:

emptyProgramInvocation {
progInvokePath = exePath,
progInvokeArgs = args,
progInvokeEnv = dataDirsEnvironmentForPlan elaboratedPlan
}

@hvr
Copy link
Member

hvr commented Jan 6, 2018

Please don't! I rely in almost all my uses of new-run of not changing the CWD

for example, I've got lots of shell wrappers in my $PATH which look e.g. like

#!/bin/sh -e

#SCRIPTPATH="$( cd "$(dirname "$0")" ; pwd -P )"
SCRIPTPATH=/home/hvr/work/GitHub/cabal-plan
EXENAME="exe:$(basename "$0")"

exec cabal new-run -v0 --project-file "$SCRIPTPATH/cabal.project" "$EXENAME" -- "$@"

and which rely on the caller's CWD to remain unaffected.

@angerman
Copy link
Collaborator

angerman commented Jan 6, 2018

Where ever this goes, let us please reduce changes to CWD rather than add even more. To stop hacking around this with hadrian and ghc, I had to start working on #4874, which tries to do exactly the opposite and stop relying on the CWD.

After all the CWD is some ugly global state. I'm convinced we should try to reduce touching it to the very bare minimum we can. It is also to my understanding the reason cabal has to shell out and call itself for parallel builds, due to the messy global CWD state.

@fgaz
Copy link
Member Author

fgaz commented Jan 6, 2018

What about the cabal tests then? Should they get the test directory as an argument? Should we add the test directory as a datadir?

(i'm not arguing against not touching CWD, I agree with that too)

edit: to reproduce:

$ cd cabal-repo-root
$ cabal-#4874 new-run cabal-install:integration-tests2

@hvr
Copy link
Member

hvr commented Jan 6, 2018

@fgaz test/benchmarks are a bit of a special case; some random ideas: have a --set-cwd flag for new-run; or alternatively, set some env-vars like CABAL_PROJECT_ROOT and CABAL_PACKAGE_ROOT or maybe others...

@23Skidoo
Copy link
Member

23Skidoo commented Jan 6, 2018

I'd prefer to not add more env vars affecting cabal-install's behaviour, because, as Zen of Python tells us, explicit is better than implicit.

@hvr
Copy link
Member

hvr commented Jan 6, 2018

@23Skidoo yeah, but how to do we communicate with an exitcode-stdio type benchmark/test-suite if we need to tell it locations or other meta-information of its environment (which is something we're gonna need)?

@ta0kira
Copy link

ta0kira commented May 6, 2020

I agree with #5001 (comment). I had a bug for a week or so that went unnoticed by my CI because the path used by getDataFileName was "coincidentally" the same as $PWD. (I was running my tests from the package path and my code had a bug where it was effectively always using $PWD instead of the data path.)

I wasn't previously aware that I could fix my testing with cd and --project-file. If run did a cd then I don't know how I'd be testing that my code uses getDataFileName in the correct places.

@fgaz
Copy link
Member Author

fgaz commented May 7, 2020

I guess this can be closed? The tests still fail if not ran from within the package directory, but that's a separate issue

lspitzner added a commit to lspitzner/ghcid that referenced this issue Jul 9, 2020
cabal repl switches current directory for subpackages of
a project, resulting in error paths relative to some
subdirectory. This confuses downstream tooling.

Related cabal tickets are
haskell/cabal#5001
haskell/cabal#1842

Gated behind the newly added flag --force-absolute-paths
lspitzner added a commit to lspitzner/ghcid that referenced this issue Jul 9, 2020
cabal repl switches current directory for subpackages of
a project, resulting in error paths relative to some
subdirectory. This confuses downstream tooling.

Related cabal tickets are
haskell/cabal#5001
haskell/cabal#1842

Gated behind the newly added flag --force-absolute-paths
@fgaz
Copy link
Member Author

fgaz commented Dec 15, 2020

I'll just close this. If someone needs tests/exes to have some additional info, they can open a new issue and link this.

@fgaz fgaz closed this as completed Dec 15, 2020
lspitzner added a commit to lspitzner/ghcid that referenced this issue Apr 16, 2021
cabal repl switches current directory for subpackages of
a project, resulting in error paths relative to some
subdirectory. This confuses downstream tooling.

Related cabal tickets are
haskell/cabal#5001
haskell/cabal#1842

Gated behind the newly added flag --force-absolute-paths
lspitzner added a commit to lspitzner/ghcid that referenced this issue Mar 14, 2022
cabal repl switches current directory for subpackages of
a project, resulting in error paths relative to some
subdirectory. This confuses downstream tooling.

Related cabal tickets are
haskell/cabal#5001
haskell/cabal#1842

Gated behind the newly added flag --force-absolute-paths
lspitzner added a commit to lspitzner/ghcid that referenced this issue Mar 1, 2024
cabal repl switches current directory for subpackages of
a project, resulting in error paths relative to some
subdirectory. This confuses downstream tooling.

Related cabal tickets are
haskell/cabal#5001
haskell/cabal#1842

Gated behind the newly added flag --force-absolute-paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants