Consistent resolution of project root w.r.t --project-file path

[pranaysashank]

• This commit sets the ProjectRoot consistently for both absolute
  and relative --project-file paths. The ProjectRoot is set as the
  directory where the --project-file is located.

• Before this change relative project files are rooted to where the
  project file is resolved, when recursively searching upward from
  the current directory, and not where it is located.

  Previous behaviour is as follows. With the following directory layout:

  gitpod ~/0/1/2/foo $ tree ~/0
  /home/gitpod/0
  └── 1
      ├── 2
      │   └── foo
      │       ├── app
      │       │   └── Main.hs
      │       ├── CHANGELOG.md
      │       └── foo.cabal
      └── cabal.project

and inside foo let's run

  $ pwd
  /home/gitpod/0/1/2/foo
  $ cabal build all --project-file ../cabal.project

which builds the project setting the project root at 2 using
the project file at 1

  $ ls ~/0/1 ~/0/1/2
  /home/gitpod/0/1:
  2  cabal.project

  /home/gitpod/0/1/2:
  dist-newstyle foo

However, if the cabal.project file was instead in 0 directory,
cabal would fail to build since the resolved project root is at
1 and cabal fails to find a package at 1/foo

  gitpod ~/0/1/2/foo $ mv ~/0/1/cabal.project ~/0/
  gitpod ~/0/1/2/foo $ cabal build all --project-file ../cabal.project
  When using configuration(s) from /home/gitpod/0/1/../cabal.project, the following errors occurred:
  The package location 'foo' does not exist.

This behaviour is almost never what we want.

See #7695

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

[fgaz]

Could you add a changelog entry? It'd be nice to have a test too, but I'm not sure if that's doable with the current test infrastructure

[pranaysashank]

I see some tests that directly use findProjectRoot function. I think we can probably write an additional test for it.

[pranaysashank]

Just an update: the fix above has a bug & is wrong when startDir is passed to findProjectRoot. (Looks like this argument is used only in the testing code and nowhere else.)

[Mikolaj]

Yes, seems to be used only for tests.

[Mikolaj]

@pranaysashank: hi! how is it going?

[pranaysashank]

Hi, I didn't get time to come back to this and it completely slipped off my radar. The PR works except in tests where the argument startDir is used, perhaps we can change current directory some other way in the tests and remove the startDir argument completely?

[Mikolaj]

perhaps we can change current directory some other way in the tests and remove the startDir argument completely?

Sure, why not. I'm sure there are explicit ways of changing directory available.

[pranaysashank]

The tests are failing because it couldn't find the directory, likely because we are running the tests from the outer directory. So, this works

cabal test integration-tests2 --test-option '-p' --test-option 'find root'

but not

cabal run integration-tests2 -- -p 'find root'

Is there a way to change directory in the tests correctly in both the scenarios?

[Mikolaj]

Wow, I had no idea one can even do

 cabal run integration-tests2

Perhaps let's just switch this to cabal test everywhere in CI scripts? Does either of

$(cabal list-bin integration-tests2) -- -p 'find root'

or

$(cabal list-bin exe:integration-tests2) -- -p 'find root'

or

`cabal list-bin test:integration-tests2` -- -p 'find root'

work fine?

[pranaysashank]

No none of those work, I need to be in cabal-install directory for any of them to work. I guess cabal test puts me in the correct working directory somehow.

In the same IntegrationTests2 file, there is also a function configureProject which calls cleanProject testdir (defined in the same file). I wonder how and if it works.

[Mikolaj]

Actually, the CI script I had in mind changes to that directory:

.github/workflows/windows.yml-          cd cabal-install
.github/workflows/windows.yml:          cabal v2-run cabal-install:integration-tests2

it's the other one that doesn't:

validate.sh:CMD="$($CABALPLANLISTBIN cabal-install:test:integration-tests2) -j1 --hide-successes --with-ghc=$HC"

Would it work to just slip cd cabal-install; before the $($CABALPLANLISTBIN?

[Mikolaj]

And you may be right that cleanProject is wrong if not called from cabal-install/, because it uses the hardwired

basedir = "tests" </> "IntegrationTests2"

and I wasn't able to find there calls to any code that goes up the directory hierarchy to find the real project root. I might have missed something, though. Anyway, if cleanProject is wrong then it's one more reason to only call the tests from cabal-install/.

[pranaysashank]

Would it work to just slip cd cabal-install; before the $($CABALPLANLISTBIN?

Yep, that should work

[Mikolaj]

Or one could try something like

validate.sh:CMD="$CABAL test cabal-install:test:integration-tests2 --test-options='-j1 --hide-successes --with-ghc=$HC'"

though I'm not sure in what order to next the quotes, etc.

[pranaysashank]

yes although --test-options usually doesn't work for me with patterns, that's why I used test-option twice and passed the flags individually in my comment above

[Mikolaj]

Yes, I have the same problem. Would that be a bug or do we lack some obvious shell trick to make it work?

[pranaysashank]

Would that be a bug or do we lack some obvious shell trick to make it work?

It's a bug in splitArgs function; I figured a way to make it work which is double quotes inside single quotes and not the other way around,
so this works

cabal test integration-tests2 --test-options '-p "find root"'

but not this

cabal test integration-tests2 --test-options "-p 'find root'"

because,

> splitArgs "-p 'find root'"
["-p","'find","root'"]

--test-options is annoying for another reason which is that every time you change them, cabal re-configures the packages in the project.

[Mikolaj]

Yay, great findings. Could you file both issues? If not, I will do that, because they are important usability hiccups and both seem reasonably easily fixable.

[pranaysashank]

I'll file them (See #8090, #8091)

[cydparser]

I've created a PR that moves towards consistency in a different way here: #8454.

[Mikolaj]

Could I ask for help reviewing the PR fixing this issue (#8454)? Even a cursory look and a comment whether this might fix your use case (bonus points for checking out and confirming) would be very much appreciated.

[pranaysashank]

@Mikolaj From reading the description and comments in that PR, it doesn't look like it's related to this issue / PR. This PR fixes the project resolution when the project file is passed as a relative path. The open question listed in the --project-dir PR (#8454) is what this PR fixes but the issue is a bit more complicated than child directory being the project root (as mentioned in the open question), the main issue was what I wrote in the second point in description of this PR

Before this change relative project files are rooted to where the
project file is resolved, when recursively searching upward from
the current directory, and not where it is located.

I haven't checked if this behaviour was fixed in a recent release / HEAD. This PR gives the same behaviour desired in the open question i.e. with this fix --project-file ../cabal.project will be equivalent to --project-dir ../

[Mikolaj]

@pranaysashank: my bad, I assumed this is one of the issues potentially fixed by #8454, while this is a PR fixing a related issue. Thank you for you remarks, though.

[Mikolaj]

A new proposal dropped at #8454. Could you kindly voice your opinion?

[ulysses4ever]

A new proposal dropped at #8454. Could you kindly voice your opinion?

#8454 has been merged. I wonder if this PR may be closed now as superseded by that one.

[Kleidukos]

@pranaysashank shall we close this PR? :)