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

Make Cabal agnostic about working directory #9718

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

sheaf
Copy link
Collaborator

@sheaf sheaf commented Feb 16, 2024

Template Α: This PR modifies cabal behaviour

This PR tackles #9702, making the library functions in Cabal agnostic to the working directory. In practice, this means that we distinguish FilePaths from un-interpreted SymbolicPaths. The latter may be paths that are relative to the working directory, and need to be interpreted with respect to a passed-in argument specifying the working directory, instead of using the working directory of the current process.
See Note [Symbolic paths] in Distribution.Utils.Path.

In particular, paths in the package description now use the SymbolicPath abstraction, which allows specifying whether they are allowed to be absolute, and, if they are relative, what they are relative to.
For example, source files are relative to a source search directory, data files are relative to the data directory, and doc files are relative to the package root.

Review notes

For review, I would recommend starting by reading the notes and comments in Distribution.Utils.Path.

The main goal is to not set the working directory when calling the Cabal library internally when building a package in cabal-install, and instead passing it separately (currently via a global flag to the Setup executable).
To enable this, the entire Cabal library needs to be able to interpret file paths with respect to a passed-in working directory, instead of using the working directory of the current process. This means all interactions with the file system (e.g. doesFileExist, rewriteFile etc), as well as all invocations of external processes (such as ghc) need to be scrutinised for correctness.


TODO:

  • More extensive testing, e.g. compiling stackage.

QA notes

The testing strategy for this patch is simply to build, test & benchmark packages. The main difference is that we no longer set the working directory when invoking the Cabal library internally (see internalSetupMethod), relying instead on passing -working-dir.

@sheaf sheaf marked this pull request as draft February 16, 2024 12:56
@sheaf sheaf force-pushed the wip/work-dir branch 20 times, most recently from 3bd7085 to 036ad97 Compare February 23, 2024 11:43
@sheaf sheaf force-pushed the wip/work-dir branch 9 times, most recently from fe901be to 2f99157 Compare February 26, 2024 13:47
@sheaf sheaf force-pushed the wip/work-dir branch 5 times, most recently from f060e50 to 42a0162 Compare March 27, 2024 10:02
@sheaf sheaf added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Mar 29, 2024
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Mar 31, 2024
This commit makes the library functions in Cabal agnostic of the working
directory. In practice, this means that we distinguish `FilePath`s
from un-interpreted `SymbolicPath`s. The latter may be paths that are
relative to the working directory, and need to be interpreted with
respect to a passed-in argument specifying the working directory,
instead of using the working directory of the current process.
See Note [Symbolic paths] in Distribution.Utils.Path.

In particular, paths in the package description now use the SymbolicPath
abstraction, which allows specifying whether they are allowed to be
absolute, and, if they are relative, what they are relative to.
For example, source files are relative to a source search directory,
data files are relative to the data directory, and doc files are
relative to the package root.

Fixes haskell#9702
@@ -46,7 +47,7 @@ licenseRaw :: Lens' PackageDescription (Either SPDX.License License)
licenseRaw f s = fmap (\x -> s{T.licenseRaw = x}) (f (T.licenseRaw s))
{-# INLINE licenseRaw #-}

licenseFiles :: Lens' PackageDescription [SymbolicPath PackageDir LicenseFile]
licenseFiles :: Lens' PackageDescription [RelativePath Pkg File]
Copy link
Collaborator

@phadej phadej Aug 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very unhappy to see these kind of changes. You don't get any benefit. The SymbolicPath PackageDir LicenseFile was already relative, it's a (relative) path from package directory to the license file.

You also drop some type-safety, as LicenseFile was telling that it's not some file, but a license file (e.g. not a some file in extraSrcFiles.

Again, I'm very unhappy to see this. You should have asked before accepting 10k diff.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, there shouldn't be any non-relative files in .cabal files. The offenders like extra-lib-dirs simply don't belong into package files; those should be configured by users externally (e.g. in cabal.project or cabal.config files). That's the historical mistake which should be cleaned up, not further cemented.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forcing users who want to install a package to turn it into a project or otherwise hack their config to make it work on e.g. MacOS isn't really acceptable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean you need to remove the support for extra-lib-dirs right away, but having system specific hard-coded paths is not correct either. The absolute paths do not belong into package definitions. That's why there are build-type: Configure, (soon Hooks) and build-type: Custom.

Copy link
Collaborator Author

@sheaf sheaf Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very unhappy to see these kind of changes. You don't get any benefit. The SymbolicPath PackageDir LicenseFile was already relative, it's a (relative) path from package directory to the license file.

You also drop some type-safety, as LicenseFile was telling that it's not some file, but a license file (e.g. not a some file in extraSrcFiles.

Again, I'm very unhappy to see this. You should have asked before accepting 10k diff.

Oleg, I'm sorry but I spent a lot of time trying to come up with a design that was workable for the whole of Cabal. To do that, I did find it necessary to change the API in several ways. For instance, yes, we no longer specify the exact kind of file:

  • it lead to a proliferation of definitions for all the different kinds of files,
  • in my experience it was not useful to keep track of which specific file something was pointing to, it was extra book-keeping that didn't in practice avoid any bugs.

One other major point of departure came from the fact I realised we unfortunately would not be able to only use relative symbolic paths everywhere; there are a lot of parts in Cabal which use paths that are either relative or absolute, so I had to make that work with the API.

I understand if you think this "cemented" bad parts of the codebase, but I want to say that it is much easier now to change these things because it is often a case of doing a search/replace, whereas when most of the codebase was "untyped" (= using FilePath) it took a lot of effort to identify the places which needed to be changed.

I know, and can tell, that you put serious consideration into the design of the initial API. I honestly did try sticking to it as much as I could but I was also informed by looking at what was actually in use in the Cabal codebase. Maybe you think that's a mistake, and that I should have stuck to a design and tried harder to make Cabal adapt to that abstract ideal, but I don't think I would have managed to write that patch with that approach.

Copy link
Collaborator

@phadej phadej Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a lot of parts in Cabal which use paths

That is the main pain point.

The SymbolicPath was intended to be used as symbolic path, i.e. paths to be interpreted that appear in .cabal files. Not any real filepath which may appear on the filesystem. That should had different type. FilePath or OsPath or ... I.e. not as typed wrapper around filepath / directory. Maybe it's not the case, but the distinction is now very blurry. (In particular, CWD root doesn't make sense for SymbolicPaths as paths in .cabal file in my design).

TL;DR I hope that SymbolicPath is not the only abstraction over paths, as in my opinion, it simply won't work well. Too many concerns, too complicated API. (Example distinction: symbolic paths are text, files on disk should eventually use OsPath as representation).

Comment on lines +24 to +32
( BuildFlags
( BuildCommonFlags
, buildVerbosity
, buildDistPref
, buildCabalFilePath
, buildWorkingDir
, buildTargets
, ..
)
Copy link
Contributor

@ulidtko ulidtko Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change to Cabal library users. 💥

import Distribution.Simple.Setup (BuildFlags (buildVerbosity))

miniRepro :: BuildFlags
miniRepro = emptyBuildFlags {buildVerbosity = undefined}

— compiles against Cabal 3.12; yields compile error with 3.14:

src/Distribution/Extra/Doctest.hs:235:13: error: [GHC-16444]
    • non-bidirectional pattern synonym ‘Cabal-3.14.0.0:Distribution.Simple.Setup.Build.BuildCommonFlags’ used in an expression
    • In a record update at field ‘buildVerbosity’
      and with pattern synonym ‘Cabal-3.14.0.0:Distribution.Simple.Setup.Build.BuildCommonFlags’.
      In the expression: emptyBuildFlags {buildVerbosity = undefined}
      In an equation for ‘miniRepro’:
          miniRepro = emptyBuildFlags {buildVerbosity = undefined}
    |
235 | miniRepro = emptyBuildFlags {buildVerbosity = undefined}
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

(edit: on GHC 9.6.4)

ffaf1 added a commit to ffaf1/cabal that referenced this pull request Nov 22, 2024
PR haskell#9718 and related PRs reshuffled and refactored Cabal API.
This patch adds a simple migration guide for Cabal library
users.

Authored-by: Maxim Ivanov
ffaf1 added a commit to ffaf1/cabal that referenced this pull request Nov 22, 2024
PR haskell#9718 and related PRs reshuffled and refactored Cabal API.
This patch adds a simple migration guide for Cabal library
users.

Authored-by: Maxim Ivanov
mergify bot added a commit that referenced this pull request Nov 30, 2024
* Remove duplicated “Other changes” subtitle

* Add migration guide for #9718

PR #9718 and related PRs reshuffled and refactored Cabal API.
This patch adds a simple migration guide for Cabal library
users.

Authored-by: Maxim Ivanov

* Update release-notes/Cabal-3.14.0.0.md

Co-authored-by: Maxim Ivanov <ulidtko@gmail.com>

---------

Co-authored-by: Maxim Ivanov <ulidtko@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants