purescript / registry-dev

Development work related to the PureScript Registry
https://github.com/purescript/registry
95 stars 80 forks source link

Legacy spago.dhall imports overconstrain package dependencies. #662

Closed natefaubion closed 9 months ago

natefaubion commented 11 months ago

The current Spago assumes that a single spago.dhall contains both src and test dependencies, by way of the default "sources" globs and test command. The current legacy importer takes these dependencies at face value, resulting in overconstrained package dependencies. For example, many of my projects are portable libraries, but their test framework relies on node dependencies. This recently caused issues when purescript-node released breaking changes across the org, which ended up booting my projects out of the package set even though they continue to compile fine given accurately constrained dependencies. I assume that this could be quite common across the ecosystem, since my project configurations are merely the default recommended setup from stable Spago.

Stable Spago is quite pedantic in the accuracy of dependencies (noting both missing and unused dependencies), so perhaps we could amend the legacy importer for spago.dhall files to prune test dependencies by doing the "unused dependency" check on src globs only. This would presumably yield a set of dependencies that are unnecessary for installing the package.

thomashoneyman commented 11 months ago

This is how spago-legacy checks for unused dependencies: https://github.com/purescript/spago-legacy/blob/11e68ece50ede286660452ef370baa3ab5c33c02/src/Spago/Build.hs#L65-L67

In the registry we produce JSON from the spago.dhall / packages.dhall alone here: https://github.com/purescript/registry-dev/blob/6a803c37577af368caa221a2a06d6be2079d32da/app/src/App/Legacy/Manifest.purs#L329-L334

And we then write that manifest as part of the publishing process here: https://github.com/purescript/registry-dev/blob/6a803c37577af368caa221a2a06d6be2079d32da/app/src/App/API.purs#L427-L437

I don't want to fetch the full project source when producing the legacy manifest, but I think we could introduce a step after fetching the legacy manifest and before writing it as a purs.json file in which we do a quick fixup with purs graph. We have access to the project source at the point we write the legacy manifest (the code above), because of this line:

https://github.com/purescript/registry-dev/blob/6a803c37577af368caa221a2a06d6be2079d32da/app/src/App/API.purs#L364

An ideal solution, to my mind, would be to write a function that can take the src directory and a legacy manifest and prune that manifest such that unused dependencies are removed. We can then write independent tests for that function and then add it as a fixup step in the publish pipeline.

f-f commented 11 months ago

@thomashoneyman we now have a whole module for making sense of purs graph - the checkImport function will collect the unused packages, which we could prune from the list we have.

thomashoneyman commented 11 months ago

Ah, that's convenient! From a brief scan it looks like unused indeed skips test globs, so it looks like we could just use checkImport directly to implement this change — or we could copy/paste the Spago checkImport implementation into the registry and trim it down to only the unused dependency part so as to avoid having to produce a fake workspace package and GraphEnv.

f-f commented 11 months ago

@thomashoneyman we can reuse this code - it's possible to have the registry-app depend on spago-lib (since spago-lib only depends on registry-core and registry-foreign)

thomashoneyman commented 9 months ago

Working on this, should have something up in the next day or so.