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.

Sign up for GitHub

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

Jump to bottom

De-duplicating buildInplaceUnpackedPackage and buildAndInstallUnpackedPackage #9499

Closed
alt-romes opened this issue Dec 6, 2023 · 1 comment · Fixed by #9524
Closed

De-duplicating buildInplaceUnpackedPackage and buildAndInstallUnpackedPackage #9499

alt-romes opened this issue Dec 6, 2023 · 1 comment · Fixed by #9524
Assignees
@alt-romes
Labels
needs triage type: bug

Comments

@alt-romes
Copy link
Collaborator

alt-romes commented Dec 6, 2023 •
edited
Loading

It seems that there are a lot of things in common between buildInplaceUnpackedPackage and buildAndInstallUnpackedPackage, however, there is also a lot of duplication between the two.

In my opinion, it would be better to factor the common bits (not changing any behaviour... best effort) and have a single function for both (similar) use cases. The subtle bugs due to the duplication and not doing things uniformly for the two situations seems much worse than possibly incurring a regression in the behaviour change, especially going forward.
@andreabedini
Copy link
Collaborator

Yes, absolutely, but let's make sure we factor out common functionality (as you say) rather than jumble them toghether. I see many cases where "different things" are forced through a same code path leaving up with a bunch of case-splitting at the bottom. I.e.

f :: A -> IO ()
f = ... (a lot of duplication)

g :: B -> IO ()
g = ... (a lot of duplication)

ends up being "simplified" as

fg :: Either A B -> IO ()
fg ab = do
  ...
  case ab of
   A -> ...
   B -> ...
  ...
  case ab of
   A -> ...
   B -> ...
  ...

rather than e.g.

f :: A -> IO ()
f = do
  common1
  specific
  common2
  specific

g :: B -> IO ()
g = do
  common1
  specific
  common2
  specific

I am sure this is what you have in mind. I just needed to get this out of my chest 😂

alt-romes added a commit to alt-romes/cabal that referenced this issue Dec 15, 2023
@alt-romes
Refactor ProjectBuilding into Package Phases
a8eb4d9 
This refactor of Distribution.Client.ProjectBuilding does the following:

* Moves package file monitoring logic to
  Distribution.Client.ProjectBuilding.PackageFileMonitor

* Moves the `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` with auxiliary functions to
  Distribution.Client.ProjectBuilding.UnpackedPackage

* Refactors the common bits of `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` to remove considerable code
  duplication while simplifying and making both functions more
  structured.

Namely, to better structure build inplace vs build and install, I've
introduced:

* `PackageBuildingPhase` describes the various phases of processing the
  unpacked package both inplace and on install
    * Configure
    * Build
    * Install (copy + register)
    * Test
    * Bench
    * Repl
    * Haddock

* Then, `buildAndRegisterUnpackedPackage` implements the common logic
  between the two functions (such as calls to ./Setup and the order of
  the phases) but delegates the logic specific to each phase to an
  argument function which maps `PackageBuildingPhase` to `IO` actions.

* Now, build inplace and build and install functions are comprised as:
    * A wrapper around `buildAndRegisterUnpackedPackage` which does
      things specific to each before and after the main phases are
      processed
    * A delegate function which maps an action to each package
      processing phase

Fixes haskell#9499
@alt-romes alt-romes mentioned this issue Dec 15, 2023
Merged
alt-romes added a commit to alt-romes/cabal that referenced this issue Dec 18, 2023
@alt-romes
Refactor ProjectBuilding into Package Phases
531e5e2 
This refactor of Distribution.Client.ProjectBuilding does the following:

* Moves package file monitoring logic to
  Distribution.Client.ProjectBuilding.PackageFileMonitor

* Moves the `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` with auxiliary functions to
  Distribution.Client.ProjectBuilding.UnpackedPackage

* Refactors the common bits of `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` to remove considerable code
  duplication while simplifying and making both functions more
  structured.

Namely, to better structure build inplace vs build and install, I've
introduced:

* `PackageBuildingPhase` describes the various phases of processing the
  unpacked package both inplace and on install
    * Configure
    * Build
    * Install (copy + register)
    * Test
    * Bench
    * Repl
    * Haddock

* Then, `buildAndRegisterUnpackedPackage` implements the common logic
  between the two functions (such as calls to ./Setup and the order of
  the phases) but delegates the logic specific to each phase to an
  argument function which maps `PackageBuildingPhase` to `IO` actions.

* Now, build inplace and build and install functions are comprised as:
    * A wrapper around `buildAndRegisterUnpackedPackage` which does
      things specific to each before and after the main phases are
      processed
    * A delegate function which maps an action to each package
      processing phase

Fixes haskell#9499
@alt-romes alt-romes self-assigned this Dec 18, 2023
alt-romes added a commit to alt-romes/cabal that referenced this issue Jan 5, 2024
@alt-romes
Refactor ProjectBuilding into Package Phases
594cd02 
This refactor of Distribution.Client.ProjectBuilding does the following:

* Moves package file monitoring logic to
  Distribution.Client.ProjectBuilding.PackageFileMonitor

* Moves the `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` with auxiliary functions to
  Distribution.Client.ProjectBuilding.UnpackedPackage

* Refactors the common bits of `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` to remove considerable code
  duplication while simplifying and making both functions more
  structured.

Namely, to better structure build inplace vs build and install, I've
introduced:

* `PackageBuildingPhase` describes the various phases of processing the
  unpacked package both inplace and on install
    * Configure
    * Build
    * Install (copy + register)
    * Test
    * Bench
    * Repl
    * Haddock

* Then, `buildAndRegisterUnpackedPackage` implements the common logic
  between the two functions (such as calls to ./Setup and the order of
  the phases) but delegates the logic specific to each phase to an
  argument function which maps `PackageBuildingPhase` to `IO` actions.

* Now, build inplace and build and install functions are comprised as:
    * A wrapper around `buildAndRegisterUnpackedPackage` which does
      things specific to each before and after the main phases are
      processed
    * A delegate function which maps an action to each package
      processing phase

Fixes haskell#9499
alt-romes added a commit to alt-romes/cabal that referenced this issue Jan 6, 2024
@alt-romes
Refactor ProjectBuilding into Package Phases
9a5f0fc 
This refactor of Distribution.Client.ProjectBuilding does the following:

* Moves package file monitoring logic to
  Distribution.Client.ProjectBuilding.PackageFileMonitor

* Moves the `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` with auxiliary functions to
  Distribution.Client.ProjectBuilding.UnpackedPackage

* Refactors the common bits of `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` to remove considerable code
  duplication while simplifying and making both functions more
  structured.

Namely, to better structure build inplace vs build and install, I've
introduced:

* `PackageBuildingPhase` describes the various phases of processing the
  unpacked package both inplace and on install
    * Configure
    * Build
    * Install (copy + register)
    * Test
    * Bench
    * Repl
    * Haddock

* Then, `buildAndRegisterUnpackedPackage` implements the common logic
  between the two functions (such as calls to ./Setup and the order of
  the phases) but delegates the logic specific to each phase to an
  argument function which maps `PackageBuildingPhase` to `IO` actions.

* Now, build inplace and build and install functions are comprised as:
    * A wrapper around `buildAndRegisterUnpackedPackage` which does
      things specific to each before and after the main phases are
      processed
    * A delegate function which maps an action to each package
      processing phase

Fixes haskell#9499
alt-romes added a commit to alt-romes/cabal that referenced this issue Jan 6, 2024
@alt-romes
Refactor ProjectBuilding into Package Phases
d6506db 
This refactor of Distribution.Client.ProjectBuilding does the following:

* Moves package file monitoring logic to
  Distribution.Client.ProjectBuilding.PackageFileMonitor

* Moves the `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` with auxiliary functions to
  Distribution.Client.ProjectBuilding.UnpackedPackage

* Refactors the common bits of `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` to remove considerable code
  duplication while simplifying and making both functions more
  structured.

Namely, to better structure build inplace vs build and install, I've
introduced:

* `PackageBuildingPhase` describes the various phases of processing the
  unpacked package both inplace and on install
    * Configure
    * Build
    * Install (copy + register)
    * Test
    * Bench
    * Repl
    * Haddock

* Then, `buildAndRegisterUnpackedPackage` implements the common logic
  between the two functions (such as calls to ./Setup and the order of
  the phases) but delegates the logic specific to each phase to an
  argument function which maps `PackageBuildingPhase` to `IO` actions.

* Now, build inplace and build and install functions are comprised as:
    * A wrapper around `buildAndRegisterUnpackedPackage` which does
      things specific to each before and after the main phases are
      processed
    * A delegate function which maps an action to each package
      processing phase

Fixes haskell#9499
Mikolaj pushed a commit to alt-romes/cabal that referenced this issue Jan 8, 2024
@alt-romes @Mikolaj
Refactor ProjectBuilding into Package Phases
0503a67 
This refactor of Distribution.Client.ProjectBuilding does the following:

* Moves package file monitoring logic to
  Distribution.Client.ProjectBuilding.PackageFileMonitor

* Moves the `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` with auxiliary functions to
  Distribution.Client.ProjectBuilding.UnpackedPackage

* Refactors the common bits of `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` to remove considerable code
  duplication while simplifying and making both functions more
  structured.

Namely, to better structure build inplace vs build and install, I've
introduced:

* `PackageBuildingPhase` describes the various phases of processing the
  unpacked package both inplace and on install
    * Configure
    * Build
    * Install (copy + register)
    * Test
    * Bench
    * Repl
    * Haddock

* Then, `buildAndRegisterUnpackedPackage` implements the common logic
  between the two functions (such as calls to ./Setup and the order of
  the phases) but delegates the logic specific to each phase to an
  argument function which maps `PackageBuildingPhase` to `IO` actions.

* Now, build inplace and build and install functions are comprised as:
    * A wrapper around `buildAndRegisterUnpackedPackage` which does
      things specific to each before and after the main phases are
      processed
    * A delegate function which maps an action to each package
      processing phase

Fixes haskell#9499
alt-romes added a commit to alt-romes/cabal that referenced this issue Jan 8, 2024
@alt-romes
Refactor ProjectBuilding into Package Phases
f70fc98 
This refactor of Distribution.Client.ProjectBuilding does the following:

* Moves package file monitoring logic to
  Distribution.Client.ProjectBuilding.PackageFileMonitor

* Moves the `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` with auxiliary functions to
  Distribution.Client.ProjectBuilding.UnpackedPackage

* Refactors the common bits of `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` to remove considerable code
  duplication while simplifying and making both functions more
  structured.

Namely, to better structure build inplace vs build and install, I've
introduced:

* `PackageBuildingPhase` describes the various phases of processing the
  unpacked package both inplace and on install
    * Configure
    * Build
    * Install (copy + register)
    * Test
    * Bench
    * Repl
    * Haddock

* Then, `buildAndRegisterUnpackedPackage` implements the common logic
  between the two functions (such as calls to ./Setup and the order of
  the phases) but delegates the logic specific to each phase to an
  argument function which maps `PackageBuildingPhase` to `IO` actions.

* Now, build inplace and build and install functions are comprised as:
    * A wrapper around `buildAndRegisterUnpackedPackage` which does
      things specific to each before and after the main phases are
      processed
    * A delegate function which maps an action to each package
      processing phase

Fixes haskell#9499
@mergify mergify bot closed this as completed in #9524 Jan 8, 2024
erikd pushed a commit to erikd/cabal that referenced this issue Apr 22, 2024
@alt-romes @erikd
Refactor ProjectBuilding into Package Phases
c67b64f 
This refactor of Distribution.Client.ProjectBuilding does the following:

* Moves package file monitoring logic to
  Distribution.Client.ProjectBuilding.PackageFileMonitor

* Moves the `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` with auxiliary functions to
  Distribution.Client.ProjectBuilding.UnpackedPackage

* Refactors the common bits of `buildInplaceUnpackedPackage` and
  `buildAndInstallUnpackedPackage` to remove considerable code
  duplication while simplifying and making both functions more
  structured.

Namely, to better structure build inplace vs build and install, I've
introduced:

* `PackageBuildingPhase` describes the various phases of processing the
  unpacked package both inplace and on install
    * Configure
    * Build
    * Install (copy + register)
    * Test
    * Bench
    * Repl
    * Haddock

* Then, `buildAndRegisterUnpackedPackage` implements the common logic
  between the two functions (such as calls to ./Setup and the order of
  the phases) but delegates the logic specific to each phase to an
  argument function which maps `PackageBuildingPhase` to `IO` actions.

* Now, build inplace and build and install functions are comprised as:
    * A wrapper around `buildAndRegisterUnpackedPackage` which does
      things specific to each before and after the main phases are
      processed
    * A delegate function which maps an action to each package
      processing phase

Fixes haskell#9499
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alt-romes alt-romes

Labels
needs triage type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor ProjectBuilding into Package Phases alt-romes/cabal
2 participants
@andreabedini @alt-romes