-
Notifications
You must be signed in to change notification settings - Fork 131
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
Rethinking Spago's Config format #842
Comments
This is also the best place/moment to propose new keys to be added to the config. So far we have a few proposals in the issue tracker:
...but feel free to add more proposals below so that we can discuss them in the context of this 🙂 |
Alright! I read this quickly on my phone, went for a long walk, then came back and read it again, thoroughly. Here are my thoughts as someone who is using spago today (to a further extent than just listing dependencies from the package set):
{ compiler : Text
, packages : Map Text Address
} instead of: { compiler : Text
, packages : {- record containing all the packages -}
} You'd be able to write: -- set without overrides
packages = upstream
-- set with overrides
packages_alt =
Spago.Index.PackageSet
{ compiler = upstream.compiler
-- assuming `//` also works for merging maps
, packages = upstream.packages // overrides
} instead of: -- set without overrides
packages =
Spago.Index.PackageSet
{ compiler = upstream.compiler
, packages = toMap upstream.packages
}
-- set with overrides
packages_alt =
Spago.Index.PackageSet
{ compiler = upstream.compiler
, packages = toMap (upstream.packages // overrides)
}
-- first, the definition of `Target` would look something like this:
let Target = {
, dependencies :
< PackageSet :
{ packages :
{ compiler : Text
, packages : Map Text Address
}
, dependencies : List Text
}
| Registry :
{ registry : Map Text Address
, dependencies : Map Text Text
}
>
-- Source globs for the local project to include in the compilation alongside its dependencies.
, sources : List Text
-- Output folder where the compiler will put its results.
, output : Text
-- A target might not be pointing at the JS backend - if that's the case we can specify here the command
-- for the alternate backend. Example values: `purerl`, `psgo`, etc.
, backend : Optional Text
}
-- these definitions below probably aren't completely correct dhall, but hopefully you can see what i'm going for
-- the definition of some target (using a package set)
Spago.Target.PackageSet::{
, sources = [ "src/**/*.purs" ]
, dependencies =
-- implying the change described in point 3. is made
{ packages = upstream
-- can't think of a better name for this field on the spot, but either this field or the other one called `dependencies` (one level up) should probably be called something else
, dependencies = [ "prelude", "effect", {- etc -} ]
}
}
-- the definition of another target (using a registry)
Spago.Target.Registry::{
, sources = [ "src/**/*.purs" ]
, dependencies =
{ registry = Spago.Index.Registry
-- can't think of a better name for this field on the spot, but either this field or the other one called `dependencies` (one level up) should probably be called something else
, dependencies = toMap { prelude = "^5.0.0", {- etc -} }
}
}
|
|
Thanks for the thoughtful comments @artemisSystem! This is all great 🙂
We'd apply the same logic that we use today for the package sets URLs: the first time we look at this file we check if these remote URLs are frozen (i.e. have a sha256 on them so that they can be cached) and if not then we freeze them. So offline would work, but not the first time.
I don't have data on this, but I feel that adding manually some dependencies to the config if we make this change would probably be slightly annoying the first times, but then quickly become a fairly mechanical operation.
Yes, it's because the I proposed a new Dhall builtin to be able to access upstream with upstream.packages.^.effect = Spago.Registry "3.0.0" ...even though the
I share your concerns and I'm torn between the two alternatives, but the reason I'm resistant about having the
Yeah, we'd specify a name for the default target and just go for that when not provided by the user, and error out if we can't find that name in the targets list. In case of multiple targets users could publish any of them...which makes me think that we need to support better the monorepo story. E.g. if you were to publish multiple packages from the same config, then one would need things like Yeah, that's already how Sorry for that, as Thomas noted that PR was still in flight at the time of writing, but now it went in so it should be clearer 😄 |
I don't have good data on it either, but the only time I can think of where I've heard folks express annoyance with Spago is when you have to manually add dependencies -- for example, when you've split out multiple I feel like I run |
These folks would not get any relief though, and adding targets would make everyone fall into this situation. E.g. I suspect that many people will want to share some dependencies between app and test targets, which is already a non-trivial-looking configuration.
I suspect that everyone got surprised/annoyed/traumatized by this because that was a change that broke everyone's builds on upgrade (if I could plan that again I would not do it in such a breaking way) and required users to install a whole lot of dependencies in all their projects all of a sudden.
Could it just be that you're used to having It could be that I spent too much time in this issue tracker, but really after all of this time of observing usage patterns and features that people ask for I am of the opinion that having So I'd say the path of "let's just leave There is probably space for a more- |
As the author of #815, I'm definitely in the "we fix it properly" camp 😉. And being in that camp, could targets be updated to include another target as one of its dependencies? I think that would deal with the corner case I found when I have to update a let binding with a new package. |
FWIW, regarding |
Coming back to this again. I don't really see a reason a library author would need/want to use a package set index. You're already specifying version bounds for your dependencies, so you don't really need to lock them down to a specific version on top of that (the version in the package set), which as i understand it, are only used for compilation anyway. As a library author myself, i would be perfectly fine with just using the registry as my package index. I think the benefits of specifying a package index per-target and not per-config, meaning that we can make package set dependencies a |
I don't have the energy for it today, but hopefully soon i can write up the pros and cons i can think of for each approach so it's easier to visualize. |
I now have a first implementation of this in a branch that I'll push asap. By how the config ended up looking, I have the feeling that making So I see two disjunct possibilities:
At this point I don't have a personal preference on this myself, so I delegate this choice to the community. We should probably run a poll on Discourse about this. |
I'd really prefer to keep Dhall and still make |
Here's another idea discussed briefly in the Discord call. Below is a more fleshed out idea for how it could work. SummaryRather than needing to edit the For example, if I write the following source code: module Main where
import Prelude
import Effect (Effect)
import Effect.Console (log)
main :: Effect Unit
main = log "hello world." Spago would automatically determine that ImplementationFirst some context:
Second, let's introduce the concept of an module-package index. A module-package file is a list of mappings of 1 module name to 1 package name. If this was saved as a text file, it might look something like this:
If it was encoded in Regardless of how the information is encoded, when CI verifies the package set, it also produces a module-package index for that package set. Third, Spago uses the module-package index to determine which dependencies are needed. Spago could determine what modules a given
Knowing what a given file's modules are, Fourth, the present design as it has been described so far creates a problem that will be resolved in the next point: how does -- src/Main.purs
module Main where
foo :: Int
foo = 1
-- test/Main.purs
module Test.Main where
import Prelude
import Effect (Effect)
import Effect.Console (log)
import Main (foo)
main = log $ show foo
-- Spago: in `test/Main.purs` for module `Test.Main`,
-- could not find a corresponding package for module `Main` Fifth and to fix the first part of point four, one must build a derivation of the base package set by adding project code to the package set as its own package(s). This derivation is called the local package set. The base package set refers to one of the released package sets from the The local package set refers to the let upstream =
https://github.com/purescript/package-sets/releases/download/psc-0.14.7-20220315/packages.dhall
sha256:708ca6d205ab5ecc1a3522e4bbe44fa576a0ebebbbc094a3aa9417be7b246974
in upstream By 'build a derivation of the base package set', I mean what we currently do with the let upstream =
https://github.com/purescript/package-sets/releases/download/psc-0.14.7-20220315/packages.dhall
sha256:708ca6d205ab5ecc1a3522e4bbe44fa576a0ebebbbc094a3aa9417be7b246974
in upstream
with language-cst-parser =
{ dependencies =
[ "arrays"
, "console"
, "..."
, "unsafe-coerce"
]
, repo = "https://github.com/natefaubion/purescript-language-cst-parser.git"
, version = "v0.10.0"
} In this proposal, one would also add their project code to the package set as its own package(s). For example, the code in the let upstream =
https://github.com/purescript/package-sets/releases/download/psc-0.14.7-20220315/packages.dhall
sha256:708ca6d205ab5ecc1a3522e4bbe44fa576a0ebebbbc094a3aa9417be7b246974
in upstream
with projectName-src = "./src/**/*.purs" as LocationGlobs
with projectName-test = "./test/**/*.purs" as LocationGlobs This approach continues to scale even with monorepos, so long as all module names in the package set are unique. let upstream =
https://github.com/purescript/package-sets/releases/download/psc-0.14.7-20220315/packages.dhall
sha256:708ca6d205ab5ecc1a3522e4bbe44fa576a0ebebbbc094a3aa9417be7b246974
in upstream
with shared-src = "./shared/src/**/*.purs" as LocationGlobs
with shared-test = "./shared/test/**/*.purs" as LocationGlobs
with backend-src = "./backend/src/**/*.purs" as LocationGlobs
with backend-test = "./backend/test/**/*.purs" as LocationGlobs
with front-end-src = "./front-end/src/**/*.purs" as LocationGlobs
with front-end-test = "./front-end/test/**/*.purs" as LocationGlobs
with scripts-codegen-api = "./scripts/codegen-api/test/**/*.purs" as LocationGlobs
with scripts-codegen-support = "./scripts/codegen-support/test/**/*.purs" as LocationGlobs Moreover, updating the Sixth and fixing the second part of part four, we introduce the concept of a base module-package index and a local module-package index. The base module-package index is the module-package index corresponding to the base package set. The local module-package index is the module-package index corresponding to the local package set. In other words, the local module-package index is the base module-package index, except that modules from packages added in the local package set take precedence over modules defined in the base package set. Why the local index has precedence over the base index is described in the next point. Seventh and introducing a new problem, what should happen when project local code defines a module name that clashes with a module name corresponding to a package defined in the base package set (an issue raised by @colinwahl in a discussion after the call finished)? For example, In other words, this is analogous to why the following script still runs. cd project
spago init
spago install
sed -i 's/module Main where/module Data.Set where/' src/Main.purs
# Notice how `ordered-collections` is never installed...
spago run Despite ConcernsIssues with offline usageBelow is a possible workflow with current spago (i.e. not how Spago works in this proposal):
This proposal may affect offline usages:
TradeoffsIDE auto-import handlingCurrently, the IDE works by importing modules based on what dependencies are installed by
This proposal flips this on its head.
Thus, the Since anything from the package set could be imported, the identifier list will be very large. As such, identifiers from local project packages should be prioritized over identifiers from packages from the base package set. Make the compiler package awareThe ideas presented so far may be better accomplished by just making the compiler package aware, such that the following syntax "just works": module Main where
import "packageName@v0.14.2" ModuleName (Foo)
import "project-local#../src/**/*.purs" ModuleName2 (bar) |
@JordanMartinez - whether that proposal works as envisioned or is incorporated in the design or not, i love the framing of it. Specifically, i think a framing of the process in a user-centric and (aspirationally) zero-config way, which is how i read your proposal, is very valuable. |
Thanks everyone for the thoughtful discussion here! It's been of great inspiration to understand the problem space a little better, and I'm grateful for that 😊 A lot of things have happened since then on many fronts - the main ones are that the Registry is now in alpha, and Spago is being rewritten in PureScript. The prototype is already merged in this repo, and we'll likely call it an alpha release as soon as we have a few more pieces working. This said, I think the purpose of this ticket might end here - the new config went through quite a few iterations and sort of settled in the current state where I'm reasonably happy with it. |
Since the inception of the upcoming Registry almost two years ago, the idea of "reimagining Spago's config" format has been on the table, because the changes in package format give us the occasion to think from scratch how a config format that takes into account all the lessons learned so far could look like.
This has been discussed in various places before, but so far there hasn't been a place to plan this rehaul, so here we are.
The new format
One of the issues of the current format is that it doesn't have a proper "Dhall type": this is because we include the package set as a Dhall record, and we don't know in advance the type of that, since the keys of the record are the package names and they might change. This has made reading the configuration a somewhat clumsy business, since we can't use the facilities provided by the
dhall
library for this, error messages are sometimes terrible, and there are issues with accepting different config formats (more on "versioning the config" later).Another issue that we have right now is that some settings sit only in the config on disk, some can only be supplied through flags, some both. We'd like to make it so that all the config options are overridable with a flag, or viceversa. Without a way to nicely evolve the config schema this work has been unpleasant and deferred.
Yet another issue of the current way we do things is that we have two configuration files (
spago.dhall
andpackages.dhall
) - this is a historical artefact of the reason why Spago exists in the first place: to allow users to create custom package sets. Now that we are revisiting the format of package sets themselves we can consolidate the two files in a single configuration, at least for simple projects.Not an issue, but a missing feature is the ability to just resolve package versions according to bounds, instead of having to use a specific pinned version as it's the case with package sets. We look at how to address this here.
The following is a first draft of how I'd like things to look like, presented in a literate-Dhall format.
In this snippet we build up a series of types from the Registry until the
Config
type.Examples of how actual configs would look like will follow in further sections, this section will only demonstrate the schema.
Without further ado:
A minimal configuration
A minimal configuration with the above schema would look like this:
Using package sets
You might note that since we're not specifying the source of our packages in the configuration above,
the default is applied, which is "the Registry" - that is, Spago is supposed to use a (not yet implemented, and not included in the scope of this yet) solver to look in the Registry for packages that match your bounds.
However, many folks would like to use package sets to not have to worry about bounds and other things.
This is how such a configuration would look like:
You may note that in this version of the config we can safely set the version of the dependencies to
*
, which meansthat we'll accept whatever version is coming from the package set.
Extending/overriding packages
Thanks to the helpers we defined above, we can easily override packages in the set with local or remote ones:
Opting in
With such a change in the config format it would be hard for us to try to support both formats in a seamless way, mostly so that we can continue providing good error messages (i.e. what happens when Spago can't match your config to either format? The error message would necessarily be very imprecise).
So I propose that we introduce a command
spago migrate
, that would allow us to do a one-time migration from the old config to the new config format, and implement a whole separate code path for the new configuration. This is cleaner, easier to maintain, and will allow us to keep our existing error messages for old configs, and add new (good) ones for the new format.Having such a clean separation also ensures that folks that are willing to try out the new config while it's still an experimental feature can do that while Spago works as before for everyone else.
Versioning the config
The question of "will we have to do all this fuss every time we want to change the schema?" comes pretty natural when discussing this.
I don't thing we would. As mentioned in the introduction, our main problem right now is that the config can't be typed with a static Dhall type, and that bars us from using the facilities to easily read things in provided by the
dhall
library.Once we can assign a type to the config, then we can just add new versions of the schema, and try to read them in until one succeeds:
Removing
spago install
Finally, I would like to propose that we just remove the
spago install $package
command.This is because supporting it properly is a lot of work already with the current format, and it might be unfeasible with the new config.
Dealing with Spago configs already requires the user to be familiar with Dhall and encourages using its features. Once folks do that (things like splitting their dependencies to different files, etc) then it becomes harder for us to support an
install
command that "just does the right thing", while for humans it would be easy (even if just slightly more unconvenient) to take care of adding dependencies in the right place.Where to go from here
This ticket contains a concrete proposal that I'll start to implement as part of the first Registry alpha, but it's also sort of an RFC and I'd like community input on this.
As a consequence of this being a first draft, things are very much in flux. The "Spago standard library" will likely be the aspect most subject to change before this stabilizes.
I have a few questions for anyone willing to read until this point:
Index
) - would we like that to be target-specific instead? So that one could have e.g. different targets building with different package setsThe text was updated successfully, but these errors were encountered: