purescript / registry-dev

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

Reimplement spago to manifest code #673

Closed JordanMartinez closed 9 months ago

JordanMartinez commented 9 months ago

Fixes the first part of https://github.com/purescript/spago/issues/1122#issuecomment-1823047315:

  1. Reimplement spago.yaml parsing in the registry (just the sections we care about)
JordanMartinez commented 9 months ago

I'm guessing I need to add yaml as an NPM dependency somewhere, but not sure how to do that.

thomashoneyman commented 9 months ago

Thanks for taking this on, @JordanMartinez! I have a few comments, and @f-f should take a look, but on the whole I think it's a good direction. I already commented on some of this, but we should add tests to the app test that parse yaml strings into the spago.yaml config format so we have some verification that our parsing and errors are good. We could also consider adding tests to the foreign workspace that just verify we're parsing YAML strings correctly so that if we update the yaml library in the future we've got assurances it's all still working.

JordanMartinez commented 9 months ago

Since I opened this PR before I read over https://github.com/purescript/registry-dev/issues/672, I'm actually thinking this is the wrong approach to take and my idea about storing purs.json file within the git tag's annotation would be a better solution.

The concerns I have here are:

I'd rather have the package managers consume their own config files and produce a purs.json file for us via some api subcommand (e.g. spago api spago-to-purs). However, that requires us to depend on (possibly multiple versions of) different package managers. So, this is also not ideal.

f-f commented 9 months ago

I'm actually thinking this is the wrong approach

I think this approach is by far the least disruptive, and my understanding is that we have reached a consensus that this is a good way to move forward. (see PR comments from Thomas anticipating having multiple config parsers explicitly)

Re your concerns:

  1. The registry is already backend-specific and linux/macOS specific, we are not aiming for wide compatibility at this point in time
  2. we'd figure out mismatches with the upstream pretty quickly as users will have their packages rejected if the parsing is not correct. Moreover, having the original config and the parser implemented here allows us to eventually reimport the package (this would go via a Trustee patch, see #80)

I'd rather have the package managers consume their own config files and produce a purs.json file for us via some api subcommand

I'd rather not. As I tried to make clear in the whole #672 thread, I am of the opinion that passing data out-of-band is a terrible idea and a good way to dig a ditch similar to the one we're trying to climb out of with the Registry.

f-f commented 9 months ago

Re this PR: it looks good so far, and I think it's good to go once Thomas' comments are addressed

thomashoneyman commented 9 months ago

I'd rather have the package managers consume their own config files and produce a purs.json file for us via some api subcommand

I'd rather not. As I tried to make clear in the whole https://github.com/purescript/registry-dev/issues/672 thread, I am of the opinion that passing data out-of-band is a terrible idea and a good way to dig a ditch similar to the one we're trying to climb out of with the Registry.

I think @JordanMartinez meant that we support Spago configs by relying on spago directly in the registry, and we call spago api spago-to-purs to generate a purs.json file rather than implement codecs in the registry code.

That said, the Spago config is simple enough that I prefer to decode it directly rather than use a CLI. (Not that this would be the case for all configs we can handle; we shell out to Dhall, for example, for spago.dhall files.)

[concern:] needing to depend on a backend-specific library (e.g. yaml).

As @f-f said, I don't consider this a problem. I can see an argument for making the registry library backend-independent, since it can be depended on directly by package managers, but the registry application explicitly runs on Node on Linux. We already bind to plenty of Node-specific libraries for that reason.

[concern:] out-of-sync issues with the package manager's config file.

we'd figure out mismatches with the upstream pretty quickly as users will have their packages rejected if the parsing is not correct. Moreover, having the original config and the parser implemented here allows us to eventually reimport the package

I agree with @f-f here that I'm not super worried about mismatches in the config parsing. If we can't parse the config then the package is rejected and it's up to the package manager to get a patch merged that fixes the config; they can always fall back to committing a purs.json file in the meantime. I'm struggling to envision a way our parsing would succeed, but the resulting manifest would be different from what the author intended.

JordanMartinez commented 9 months ago

Ok, but I'll have to get to this another time. Likely not today.

thomashoneyman commented 9 months ago

Sounds good, @JordanMartinez. I'm happy to pair on this to get it through quickly, if you'd like.

thomashoneyman commented 9 months ago

Here's the tracking for adding files (now includes/excludes) and owners to our spago.yaml parsing — https://github.com/purescript/registry-dev/issues/594

thomashoneyman commented 9 months ago

cc: @JordanMartinez this is ready for a look if you'd like to verify it.

JordanMartinez commented 9 months ago

@thomashoneyman Looks good to me!