Use Directory.canonicalizePath to resolve --with-compiler

[elland]

Passing a relative path to --with-compiler didn't canonicalise it, storing the relative path itself, causing issues with Cabal tried to invoke the compiler from a different working directory.

Should close:

#3411
#758

---

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.
• Manual QA notes have been included.
• Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

[elland]

I'd like to have at least 1 test corroborate this but I'm out of time right now. I'll try and resume this later today.

[Kleidukos]

@elland Thank you for this contribution, it was nice to pair with you on this. :)
I'll let you fill the checkboxes in the description, and we'll see if CI approves of this change!

[Mikolaj]

The PR looks fine to me, but to be merged it absolutely needs the little changelog file.

A test and a QA instruction would be needed as well, but if backporting this is crucial, they can be postponed to another PR that won't be backported (and ideally the other PR would be ready before release is over so that we can get the extra assurance this PR is fine from the test. Then this PR would be expedited (shortened waiting period before merge). What do you think?

[Mikolaj]

Are there any other places in cabal codebase where canonicalizePath or anything equivalent is called? Should there be more places? Is there a way to make it more likely that all places that need it have it, e.g., defining a function that does both listHcPath and canonicalizePath and partially hiding or deprecating listHcPath , if that's the context where many of the canonicalizePath calls (should) happen.

[elland]

@Mikolaj Unfortunately I'm not familiar enough with the codebase to answer this accurately.

[Kleidukos]

yeah there's a fair amount of places where canonicalizePath and canonicalizePathNoThrow are used in cabal-install.

Should there be more places?

no idea yet

Is there a way to make it more likely that all places that need it have it

All CLI option data types should be built with it, yes. I feel like this is another PR however.

[Mikolaj]

I feel like this is another PR however.

That's a good point. Maybe we could open a ticket so that we don't forget?

[philderbeast]

Using canonicalizePath helped with #9578.

cabal/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs

Lines 153 to 229 in 50d5e31

	-- | Normalizes and canonicalizes a path removing '.' and '..' indirections.
	-- Makes the path relative to the given directory (typically the project root)
	-- instead of relative to the file it was imported from.
	--
	-- It converts paths like this:
	-- @
	-- └─ hops-0.project
	-- └─ hops/hops-1.config
	-- └─ ../hops-2.config
	-- └─ hops/hops-3.config
	-- └─ ../hops-4.config
	-- └─ hops/hops-5.config
	-- └─ ../hops-6.config
	-- └─ hops/hops-7.config
	-- └─ ../hops-8.config
	-- └─ hops/hops-9.config
	-- @
	--
	-- Into paths like this:
	-- @
	-- └─ hops-0.project
	-- └─ hops/hops-1.config
	-- └─ hops-2.config
	-- └─ hops/hops-3.config
	-- └─ hops-4.config
	-- └─ hops/hops-5.config
	-- └─ hops-6.config
	-- └─ hops/hops-7.config
	-- └─ hops-8.config
	-- └─ hops/hops-9.config
	-- @
	--
	-- That way we have @hops-8.config@ instead of
	-- @./hops/../hops/../hops/../hops/../hops-8.config@.
	--
	-- Let's see how @canonicalizePath@ works that is used in the implementation
	-- then we'll see how @canonicalizeConfigPath@ works.
	--
	-- >>> let d = testDir
	-- >>> makeRelative d <$> canonicalizePath (d </> "hops/../hops/../hops/../hops/../hops-8.config")
	-- "hops-8.config"
	--
	-- >>> let d = testDir
	-- >>> p <- canonicalizeConfigPath d (ProjectConfigPath $ (d </> "hops/../hops/../hops/../hops/../hops-8.config") :| [])
	-- >>> render $ docProjectConfigPath p
	-- "hops-8.config"
	--
	-- >>> :{
	-- do
	-- let expected = unlines
	-- [ "hops/hops-9.config"
	-- , " imported by: hops-8.config"
	-- , " imported by: hops/hops-7.config"
	-- , " imported by: hops-6.config"
	-- , " imported by: hops/hops-5.config"
	-- , " imported by: hops-4.config"
	-- , " imported by: hops/hops-3.config"
	-- , " imported by: hops-2.config"
	-- , " imported by: hops/hops-1.config"
	-- , " imported by: hops-0.project"
	-- ]
	-- let d = testDir
	-- let configPath = ProjectConfigPath ("hops/hops-9.config" :|
	-- [ "../hops-8.config"
	-- , "hops/hops-7.config"
	-- , "../hops-6.config"
	-- , "hops/hops-5.config"
	-- , "../hops-4.config"
	-- , "hops/hops-3.config"
	-- , "../hops-2.config"
	-- , "hops/hops-1.config"
	-- , d </> "hops-0.project"])
	-- p <- canonicalizeConfigPath d configPath
	-- return $ expected == render (docProjectConfigPath p) ++ "\n"
	-- :}
	-- True
	canonicalizeConfigPath :: FilePath -> ProjectConfigPath -> IO ProjectConfigPath

[Mikolaj]

This now looks fine to me and could be conditionally merged even now, if test and QA are done in a forthcoming new PR. Let me set the merge label to put things in motion.

[elland]

There's another underlying issue preventing this from actually solving the problem, more investigative work is necessary.

[mpickering]

Am I misunderstanding or is this just affecting the listAction code path? It seems quite likely to me that this PR isn't modifying a very precise place yet.

I would advocate not backporting to 3.12 (it is not fixing a new regression and not obviously correct to me).

[Kleidukos]

I would advocate not backporting to 3.12 (it is not fixing a new regression and not obviously correct to me).

Noted, your opinion will be reported at our next meeting.

[Mikolaj]

Copying here the link to a related PR @Kleidukos has mentioned in the Matrix room: #9718. This may help writing the test and/or extending the PR.

[mpickering]

Can you explain how modifying listAction makes --with-compiler work in all the other situations it can be used? build, test, in a project file etc.

I think the patch is also missing tests for this situation.

I think the right place to canonicalise (if at all) is right before the path is passed to onwards to ./Setup or another program. It seems like #9718 might be relevant to why this didn't used to work, at what point do you get an error now? Perhaps #9718 is incorrectly offsetting the relative with-compiler path to the directory of the package.

[elland]

After more investigation my current understanding is that it would be necessary to rework how we currently compute and handle flags to fix it. Since we have no way to canonicalise the path within the flag setter itself and by the time we can, cabal has already processed the working directory flag, breaking the canonicalisation.

For haddock we're looking into reorganising the directory structure to bypass this issue for now.