Collection of patches to do with --working-dir #10256
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpickering
force-pushed
the
wip/working-dir-path-mp
branch
6 times, most recently
from
70c4f21 to
97ff932
Compare
sheaf
reviewed
Aug 16, 2024
|
Does it fix any problems in 3.12? If so, would it be backportable as a whole or only some commits? |
|
It would need to be completely rewritten to be backported, as it's necessarily heavily dependent on #9718. |
|
The patch doesn't fix any issues present in 3.12. |
5 tasks
mpickering
force-pushed
the
wip/working-dir-path-mp
branch
from
97ff932 to
bf42643
Compare
sheaf
reviewed
Aug 20, 2024
mpickering
force-pushed
the
wip/working-dir-path-mp
branch
2 times, most recently
from
812d1e8 to
3d350de
Compare
sheaf
approved these changes
Aug 21, 2024
5 tasks
geekosaur
reviewed
Aug 28, 2024
geekosaur
approved these changes
Aug 28, 2024
It is only on Cabal 3.13 that the symbolic path abstraction was introduced. On CI we only test with Cabal master so the incorrect bound wasn't picked up.
This fixes a simple oversight where the flags were passed without updating the `--working-dir` argument appropiately.
This fixes the --gen-pkg-config to use the symbolic path abstraction, in turn this ensures that we interpret the path appropiately when `--working-dir` is also set.
This refactoring enforces a simple property * We use symbolic paths in Cabal in order to represent that paths to package databases. These paths is relative to the package root. * We use normal filepaths in cabal-install to represent the path to a package database. These are relative to the current working directory. Paths are explicitly converted from one type to the other at the interface of `cabal-install` and `Cabal`, see `setupHsConfigureArgs` for where this happens. In order to achieve this `PackageDB` is abstracted over what the type of filepaths a specific package db points to. ``` type PackageDBX fp = ... | SpecificPackageDB fp | ... ``` If you are using the Cabal library then you probably want to migrate to use `PackageDBCWD` and `PackageDBStackCWD`. ``` type PackageDBCWD = PackageDBX FilePath type PackageDBStackCWD = [PackageDBCWD] ``` Then at the point where you call commands in the `Cabal` library convert these paths into paths relative to the root of the relevant package. The easiest way to do this is convert any paths into an absolute path. This patch fixes a double interpretation issue when the `--working-dir` option was used and package db paths were offset incorrectly.
There are a few places where paths are known to be absolute. This enforces that in the type system by introducing a simple wrapper to `Distribution.Utils.Path`. ``` newtype AbsolutePath (to :: FileOrDir) = AbsolutePath (forall from . SymbolicPath from to) ``` The nice thing about this abstraction is when when a path is unwrapped, due to the universally quantified `from` type, the resulting `SymbolicPath` can be used with any API which expects a path to point `from` a specific directory.
runDbProgram doesn't take into account the working directory (so will normally produce incorrect results when used in `Cabal`). This replaces the last uses which weren't found by testing, they were found by grepping.
When testing `./Setup` only, when `withDirectory` is used, instead of changing into that directory when invoking processes, we now use the `--working-dir` flag and keep a fixed CWD. This will therefore passively test that `--working-dir` is working In addition, it makes it possible to test things easily such as `--working-dir` with a relative path as an argument. `cabal-install` will only invoke `--working-dir` with an absolute path and hence is isolated from any double interpretation issues. Testing against these double interpretation issues is very important as it also prevents over-interpretation of relative paths into absolute paths. Passing absolute paths to tools such as hsc2hs can lead to the build directory leaking into an interface file which leads to non-reproducible results.
Before the previous patches this test failed because an incorrect path was passed to hsc2hs (a preprocessor), it now succeeds :)
mpickering
force-pushed
the
wip/working-dir-path-mp
branch
from
3d350de to
addcd41
Compare
mergify
bot
added
ready and waiting
|
On today's Cabal meeting, we discussed what could let this API-changing PR stay under the radar of 3.14.0.0's release notes. Unanimously people cited the absence of the PR template. We'll try to be more diligent about that. But we also ask the contributors to keep the template (and in particular choose one of the two templates) when submitting PRs. |
|
There are ways to cut PRs that don't take the template into account, notably the Git integration in VSCode/VSCodium. I think I've encountered a few others but don't recall offhand what they were, just that they involved third party tooling instead of the GitHub website. |
|
@geekosaur that's a great point, I didn't think about it. I myself have dabbled with Magit Forge, which is an Emacs extension allowing, among other things, to create PRs. Oh well... I guess, the reviewers will have to pay more attention if we want a better future. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See individual commits for details