gbuild and buildOrReplLib contain large amount of duplication #9389

mpickering · 2023-11-02T17:33:50Z

In Distribution.Simple.GHC the buildOrReplLib and gbuild are essentially duplicates of each other. The fact they are not identical leads to bugs such as #8639.

I will combine together the implementations.

The text was updated successfully, but these errors were encountered:

philderbeast · 2023-11-02T20:16:26Z

I will combine together the implementations.

Are you self-assigning this @mpickering? I'd be happy to help as I like these kinds of clean up tasks.

arjunkathuria · 2023-11-05T12:09:35Z

Hi !

I'm looking to make my first! contribution to Cabal after being encouraged by @angerman

Since this issue has a pr-welcome tag, wondering if help is still required with this.
If it's covered, I'd be happy to take another one off of your hands.

mpickering · 2023-11-06T10:17:24Z

In some ways this task is simple, but great care should be taken to get this right as this code is executed on essentially every invocation of cabal-install. I would suggest for a first issue it's quite high-risk.

arjunkathuria · 2023-11-06T11:04:40Z

Noted, Matthew.

i'll try working on something that's a better fit for now.
#9298 seems promising.

feel free to tag me on other relevant issues. Happy to help where i can : )

alt-romes · 2023-11-16T19:38:21Z

@philderbeast I accidentally looked a bit into this duplication today because I thought it would be a good refactor when I was working on #7339, not knowing about this ticket's existence.

I took a look at what needed to be done and wrote down some comments, which might be useful in your tackling this ticket. The commit with these notes is ded79d4.

In the meantime I'm going back to #7339. Good luck and thanks!

philderbeast · 2023-11-20T16:39:18Z

@mpickering and @alt-romes, the duplication is a bit more widespread. Within Cabal/src/Distribution/Simple/GHC.hs there is duplication between buildOrReplLib and gbuild as mentioned but there are also the same named functions in Cabal/src/Distribution/Simple/GHCJS.hs.

@luite and @hsyl20 do you think we could or should deduplicate buildOrReplLib and gbuild there too?

hsyl20 · 2023-11-21T10:24:33Z

@luite and @hsyl20 do you think we could or should deduplicate buildOrReplLib and gbuild there too?

No strong opinion but GHCJS is unmaintained and people should be encouraged to switch to GHC's JS backend. Hence Cabal support for GHCJS might be dropped at some point and this code removed, so there isn't really a need to improve it.

alt-romes · 2024-01-08T15:31:05Z

@philderbeast are you still working on this? I'd be happy to take it on.

philderbeast · 2024-01-08T17:42:50Z

@alt-romes, happy to work with you on it. I'd pushed it back behind some higher priority changes.

alt-romes · 2024-01-08T17:43:48Z

@alt-romes, happy to work with you on it. I'd pushed it back behind some higher priority changes.

Neat! How do you propose we work together?

philderbeast · 2024-01-08T17:58:22Z

Do you have an idea for how to add tests, #9409 (comment)?

Do you prefer the dual code paths approach, #9409 (comment)?

It is now very easy to see that there is much in common between the two modules:

Cabal/src/Distribution/Simple/GHC/BuildGeneric.hs
Cabal/src/Distribution/Simple/GHC/BuildOrRepl.hs

Here's a snippet of the diff:

alt-romes · 2024-01-08T18:12:05Z

@philderbeast

Testing in general this kind of change is hard, I don't think there will be any test that can remain in the source tree specific to this change by the time we are done (modulo bugs we may uncover).

My idea for testing the migration is not completely fleshed out, but it should boil down to building a lot of packages (eg the whole testsuite, or stackage) in verbose mode with the tree before the change and after the change, and diffing. The only differences should be ones that are purposefully and mindfully created, not accidents in the refactor.

I definitely do not want to keep the two code paths.

Having the two modules separate is nice! Thanks.

philderbeast · 2024-01-08T18:15:20Z

@alt-romes, from the diff snippet, it appears that some of the differences are naming differences. Did the names drift apart but refer to the same things? If so, then these could either be made to be the same in-place (by generalizing or picking the better name), or those common sections with naming differences could be pulled out into helper functions.

alt-romes · 2024-01-08T18:20:17Z

@alt-romes, from the diff snippet, it appears that some of the differences are naming differences. Did the names drift apart but refer to the same things? If so, then these could either be made to be the same in-place (by generalizing or picking the better name), or those common sections with naming differences could be pulled out into helper functions.

Yes, some of the differences are simply naming issues.
It would be more productive to arrange a call, if that is in your interest. I've pinged you on matrix.

Refactors the duplicated `buildExtraSources` function from `gbuild` and `buildOrReplLib` into a standalone monadic computation in the context of building a component (namely, a 'BuildM' action). This refactor allows us to share the code for building an extra source amongst the two functions, and paves the way to fixing haskell#9389.

Refactors the duplicated `buildExtraSources` function from `gbuild` and `buildOrReplLib` into a standalone monadic computation in the context of building a component (namely, a 'BuildM' action). This refactor allows us to share the code for building an extra source amongst the two functions, and paves the way to fixing haskell#9389. A standalong part of the refactor of gbuild and buildOrReplLib (haskell#9389)

This is the third part of the refactor of gbuild and buildOrReplLib (haskell#9389). It re-works the linker invocation, focusing on preserving existing behaviour before simplifying any further. Follows the spirit of the two previous commits, with the end goal of (haskell#9389) Format Improve, fix. Fix last bug in testsuite Formatting worries.. about vanilla vs static Fix one more bug Vanilla being different from Static is kind of problematic Some more compat fixes The forgotten forsaken Utils ; I accidentally didn't commit this and git cleaned my tree, which deleted this file. Some unfortunate accident had placed a "build/" entry in my global git ignore, which made me miss the file I had added under GHC/Build/... After deleting the file, I ended up finding the .swp file with which I should be able to recover the lost file, but I accidentally deleted that swap file too because of some weird interaction between my newly created Build/Utils file (created after admitting defeat) and the original one in the .swp file, which was ultimately overwritten or something ;P. Format del prints

This is the third part of the refactor of gbuild and buildOrReplLib (haskell#9389). It re-works the linker invocations, focusing on preserving existing behaviour before simplifying or fixing bugs any further. Follows the spirit of the two previous commits, with the end goal of (haskell#9389)

Refactors the duplicated `buildExtraSources` function from `gbuild` and `buildOrReplLib` into a standalone monadic computation in the context of building a component (namely, a 'BuildM' action). This refactor allows us to share the code for building an extra source amongst the two functions, and paves the way to fixing haskell#9389. A standalong part of the refactor of gbuild and buildOrReplLib (haskell#9389)

Creates a new module Distribution.Simple.GHC.Build.Modules which, in the same spirit as ...GHC.Build.ExtraModules, defines a 'BuildM' action which builds all the Haskell modules of the component being built. This function clarifies and re-implements the logic of building Haskell modules in the different possible ways, while accounting for Template Haskell special "way requirements", which was previously duplicated in a non-obvious manner in gbuild and buildOrReplLib. The Note [Building Haskell modules accounting for TH] in that module explains the big picture, and the implementation is re-done in light of it. A standalone part of the refactor of gbuild vs buildOrReplLib (haskell#9389)