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

Refactor ProjectBuilding into Package Phases #9524

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

alt-romes
Copy link
Collaborator

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 #9499

@alt-romes
Copy link
Collaborator Author

Here we are @andreabedini

@andreabedini
Copy link
Collaborator

@alt-romes I'll look at this ASAP <3

@alt-romes
Copy link
Collaborator Author

alt-romes commented Dec 18, 2023

@andreabedini I'm quite happy with how it turned out. FWIW I suggest reviewing locally with --color-moved because not that much changed.

Oh, wait! I've been debugging locally 2 failures that I couldn't really reproduce for the past hour... but CI is green.
I'm going to check again if all is fine here considering that the same tests pass on CI. Probably something is out of date.

EDIT: The failures are unrelated to the patch since I can reproduce them on master.

We're good to go!

@alt-romes
Copy link
Collaborator Author

alt-romes commented Dec 18, 2023

@andreabedini the commit message is probably helpful in guiding the review.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 4, 2024

@alt-romes: is this ready for review? If so, could you stick the needs-review label on it?

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

@alt-romes: thank you. I think --color-moved doesn't quite work for me now due to the conflicts. But from a quick glimpse, the PR looks good.

@alt-romes
Copy link
Collaborator Author

I've rebased on master and solved the conflict @Mikolaj, it should be good to read now.
@andreabedini could you also take a look?

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Thanks!

The only issue is the extra file, I think. (See below)

@alt-romes alt-romes added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Jan 6, 2024
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 8, 2024
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 merged commit 1a5b735 into haskell:master Jan 8, 2024
49 checks passed
@andreabedini
Copy link
Collaborator

@alt-romes Apologies for the radio silence. Holidays arrived and I missed the boat. This is fanstastic work ⭐

@alt-romes
Copy link
Collaborator Author

alt-romes commented Jan 17, 2024

Thanks @andreabedini! Glad you are content with it.

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.

De-duplicating buildInplaceUnpackedPackage and buildAndInstallUnpackedPackage
4 participants