search

zeme-wana / iogx

0 stars 0 forks source link

Interface review of eff3eea #1

Closed shlevy closed 1 year ago

shlevy commented 1 year ago

Review of the codebase as of eff3eea. Note that I have only reviewed the interface/documentation, not the details of implementation.

The essence of my feedback is: What we have is a brittle framework, what we want is a composable library.

Speaking abstractly:

A composable library is built by clearly defining units of functionality that can be reasoned about as a single element of thought, with precise semantics and specific delimited scope, and more complex units built out of lower level ones. Each unit should have a simple declarative/denotational interface (describing what the user wants), with any imperative/operational aspects (describing how the user wants to achieve whatever he wants) primarily as "escape hatches" whose use we aim to minimize over time by improving the main interface. A complex unit should be implemented in terms of simpler units using the same interfaces that the user has access to. Interfaces should be defined in terms of the domain appropriate for the use case, not in terms of implementation details (including implementation details of simpler units used under the hood). The end result is an integrated system that the user can reason about at any level of abstraction, whether just the high level big picture interface or the individual building blocks it's built out of, and construct his own solutions from these pieces, integrating them into the rest of his work.

When solving a complex problem without this, what inevitably results is a brittle framework. There aren't well-defined abstractions or layers, and so everything ends up blending together into a monolithic blob with varying combinations of ever-growing complex interface and overly restrictive opinionation; the developer finds himself either needing to anticipate every user need and somehow accommodate it, or limiting which use cases he cares about to those that happen to fit the uses he originally designed for. The internals of the system all get very intertwined and cannot be readily reused. The end user must reshape his other work to fit the framework, and either limit his uses to the narrow path of places where everything "just works" or delve into arcane incidental complexity when he needs to step outside of the basic boundaries.

Applying this to IOGX:

In IOGX, everything is defined through a single grab-bag top-level attribute set, without clarity about how everything interrelates or which parts one needs to think about for which purpose or what one should do if the default capabilities don't quite fit the needs. Users have to write complex functions (e.g. preparing a call to cabalProject' themselves), boilerplate is stored in templates (which will go stale), the system is pretty heavily "all or nothing". Internally, flakeopts is passed around everywhere, and so every component can (and, inevitably, will) borrow implicitly from the interface of other components. The interface is more a copied around template file than it is functions.

What I would like to see instead is clearly defined (and separately documented!) components. We might have a notion of "a Haskell project", defined by the location of a cabal.project and a set of declarative options needed on top of it. We might have a notion of a "developer shell", defined by a set of packages and scripts and background services. We might combine these to create a notion of a "Haskell project developer shell", taking a haskell project input and a set of extra packages etc. and producing a shell suitable for building the project. We might have an overall notion of "an IOG project", taking a source repo which has a bunch of declarative files[^yaml] at well known (perhaps overrideable) locations in the repo defining a Haskell project, a shell, CI, etc., in terms of the lower-level components. Each of these components would have its own interface and be usable on its own as well as in combination, and each composition would use the public interfaces internally. End users would fully understand the files that they maintain in their repos, without magic boilerplate or "here be dragons" sections that they need an expert's help with.

[^yaml]: Instead of separate files, this could just be nested attribute sets or whatever within one master file. The benefit of the former is that you can narrow in on just the capability in question and separately think about how they're composed, and then later if you decide you don't want to take the high-level composition you can still reuse the sub-component definitions.

Details

Below follows a detailed file-by-file review (for files constituting the interface), including some feedback going beyond what's above and others that repeat it in specific application

README

zeme-wana commented 1 year ago

The essence of my feedback is: What we have is a brittle framework, what we want is a composable library.

I've tried to keep things as simple as possible, hence why we have one single config attrset defining the interface (flakeopts) and one single function mkFlake. Features can be turned on/off by setting flakopts values accordingly (as opposed to including separate library function calls, see example below).

I don't want to overengineer this (the scope is not that large) and I don't want to make it more complicated than it needs to be. Also I want to merge something ASAP in serveral repositories so we can collect feedback from the users and iterate based on that, rather than on what we think is right. That's also why all documentation is gathered in the comments in flake.nix, so that the devs will actually be forced to read it if they want to make changes.

A more library-like, components-made-explicit alternative could look like this:

outputs = { iogx, ... }: 
    iogx.lib.merge [
        (myExistingFlake)
        (iogx.lib.addPerSystemOutputs { params }) 
        (iogx.lib.addSystemIndendentOutputs { params }) 
        (iogx.lib.addHaskellProjectOutputs { params })
        (iogx.lib.addDevShells { params })
        (iogx.lib.addReadTheDocsOutputs { params })
        (iogx.lib.addHydraJobs { params })
    ]

The example above is difficult to implement in practice: the params will overlap and each function will need to know about the other functions in order to work properly internally. mkFlake ends up being a thin layer threading the global configuration (flakeops) to various (internal) function which look similar to those add* in the example above. (The problem of flakeops being exposed to the user is addressed in the comments further down).

The example above identifies 6 components:

  1. General flakeutils-like system dependent outputs
  2. General top-level system-independent outputs
  3. A haskell project
  4. A developer shell
  5. Support for read-the-docs
  6. A hydra jobset

What I've found in practice is that these components are by nature intertwined, somewhat overlap, and are most useful when used in an "all or nothing" way (to an extent). I'm really not sure that it's worth forcing a separation at the function level or at the flakeops level. However, we could add a little more structure like so (though this is cheating: we are just moving from a flat hierarchy to a nested attrset):

{
    inputs = inputs;
    debug = true;
    flakeOutputsPrefix = "";
    repoRoot = ./.;
    systems = [];

    perSystemOutputsFile = {};

    systemIndendentOutputsFile = {};

    haskellProject = {
        compilers = [];
        defaultCompiler = "ghc8107";
        crossSystem = null;
        nixFile = ./haskell-project.nix;
    };

    shell = {
        enable = true;
        prompt = "";
        welcomeMessage = "";
        nixFile = ./shell.nix;
    };

    hydraJobs = {
        enable = true;
        blacklisted = [];
        enablePreCommitCheck = false;
    };

    readTheDocs = {
        enable = false;
        dir = ./docs;
        haddockPrologue = "";
        extraHaddockPackages = [];
    };
}

Having said that, another alternative to all this comes to mind. Instead of a mkFlake function that takes a top-level attrset config, we can go filesystem-based all the way:

flake.nix:

{
  outputs = inputs: inputs.iogx.lib.mkFlake inputs;
}

Now we demand a top-level ./nix folder with 7 files:

1. ./nix/iogx-config.nix
2. ./nix/per-system-outputs.nix
3. ./nix/system-independent-outputs.nix 
4. ./nix/haskell-project.nix
5. ./nix/shell.nix
6. ./nix/read-the-docs.nix
7. ./nix/hydra-jobs.nix

These files (except iogx-config.nix) must be functions that return something needed by iogx.

1 ./nix/iogx-config.nix

Contains those config values that are global or shared by several components.

{
    debug = true;
    flakeOutputsPrefix = "";
    repoRoot = ./.;
    systems = [];
    compilers = [];
    defaultCompiler = "ghc8107";
    crossSystem = null;
}

2 ./nix/per-system-outputs.nix

Same as current interface, except that flakeops disappears.

If this is not needed, the empty attrset can be returned.

{ inputs, systemized-inputs, pkgs }:
{}

3 ./nix/system-independent-outputs.nix

If this is not needed, the empty attrset can be returned.

{ inputs, systemized-inputs }:
{}

4 ./nix/haskell-project.nix

Compared to the current interface, we ca remove flakeopt, ghc, enableProfiling. And we can also hide the call to cabalProject'. And instead of returning a h.nix project, we require that the user return an attrset containing h.nix specific values that iogx will feed internally to cabalProject'. This file it not optional.

{ inputs, systemized-inputs, pkgs, deferPluginErrors }:
{ 
    shaMap = {};
    cabalConfig = "";
    modules = {};
}

5 ./nix/shell.nix

The new interface does not need flakeopts. Also, shellPrompt and shellWelcomeMessage can be moved here from the top-level. If no augmentations to the shell are needed, enabled can be set to false.

{ inputs, systemized-inputs, pkgs, haskell-nix-project }: 
{ 
    enabled = true;
    prompt = "";
    welcomeMessage = "";

    scripts = {};
    packages = [];
    env = {};
    shellHook = "";  
    processes = {}; # TODO (not trivial, will add when actually needed)
}

6 ./nix/read-the-docs.nix

Relevant config values can be moved here from flakeopts.

If read-the-docs is not supported, enabled can be set to false.

{ inputs, systemized-inputs, pkgs, haskell-nix-project }:
{   
    enabled = true;
    siteDir = ./docs;
    haddockPrologue = "";
    extraHaddockPackages = _: {};
}

7 ./nix/hydra-jobs.nix

Note that several config values that used to be in flakeops can be moved here from the top-level. If hydra jobs are not needed, enabled can be set to false.

{ inputs, systemized-inputs, pkgs, haskell-nix-project }: 
{   
    enabled = true;
    buildSystems = [];
    excludeProfiledHaskell = true;
    blacklistedJobs = [];
    enablePreCommitCheck = true;
}

Note that various escape-hatches can be implemented by means of additional fields in the attrsets returned by each of these 7 files. They can be added as neede.

In IOGX, everything is defined through a single grab-bag top-level attribute set, without clarity about how everything interrelates or which parts one needs to think about for which purpose or what one should do if the default capabilities don't quite fit the needs.

I think that due to the scope of this, a grab-bag top-level attribute set could be the right abstraction, at least until we find that new repositories require feature that make the scope larger and more complex to the point where an explicit separation of components at the interface level becomes a better solution.

Users have to write complex functions (e.g. preparing a call to cabalProject' themselves),

See comments below: this can be simplified but not totally. Functions are unavoidable if we want to expose values to the users to play with (e.g. inputs, pkgs)

boilerplate is stored in templates (which will go stale), the system is pretty heavily "all or nothing".

Templates can go stale, however they are very handy: you can run nix flake template --init and get started immediately. I'm not sure we should give up on them yet. The system only appears to be "all or nothing": out of the 6 components identified above, only 1 is required: the haskell project. devShells, hydra, readTheDocs, perSystemOutputs, globalOutputs can be turned on/off by setting or nulling the relevant flakeops fields. Granted, this approach is not ideal and a better alternative will be given, for example by means of a general enable field for each component.

Internally, flakeopts is passed around everywhere, and so every component can (and, inevitably, will) borrow implicitly from the interface of other components. The interface is more a copied around template file than it is functions.

Internally it must be, but it turns out that it does not have to be exposed to the user. The files per-system-output.nix, haskell-project.nix and shell-module.nix do not need to know about flakeopts, and so it can be removed from the interface.

README

We should have standalone documentation of various components, not just comments in templates. Would be good for each component to document a fuller inventory of "what you get" from it. E.g. "the default flake includes code formatters and a dev shell that can build your Haskell project" What is "a standard environment"? Do you mean a shell? But doesn't iogx do more than that? Should have links for setting up Nix, ideally with our caches. OK to reuse upstream docs (or maybe h.nix's).

Agreed to all this, the readme needs to be revised and expanded. I would still put off writing shiny and thorough documentation until we get user feedback and have a real v1.

flake.nix

Should we be putting things in a top-level lib output?

Yes, it makes sense and seems to be convention.

Can we get documentation for what is exported?

Yes, I will include that in the docs.

template/flake.nix

Would be good to link to some general flake docs explaining what a flake is and how the flake CLI works

Noted.

For input overrides, we should consider (or perhaps present the difference between) overriding in flake.nix with follows or in flake.lock with a nested --update-input. The latter means that the next time we update the top-level input we will lose the nested override, which may be good (since the top-level input may in fact need something newer/different) or bad.

Noted. Documenting the various ways to "play" with flake inputs will be useful information to the users.

When might I need to override some input?

When wanting to use a different version of an input managed by iogx. I will include this in the docs.

How can I know if some input is included in iogx vs can just be added top-level?

The inputs included in iogx are listed in the comments in flake.nix. One can also do nix flake info github:zeme-wana/iogx. I will include this in the docs.

The "systemized"/"desystemized" terminology is never fully explained

Much like flake-parts has inputs and inputs', we have inputs and systemized-inputs. I will improve the docs for this.

Might help to document what description is a description of

Noted.

With separate docs I don't think we should have fields with sane defaults in the template

I dislike implicit defaults in configurations because they hide information from me. For each field, I want to know what it does and what its default value is. If we use a template, we have a few choices:

  1. (best IMO) 
    # The nonempty list of supported systems.
systems = [ "x86_64-linux" "x86_64-darwin" ];
  2. (one character longer than 1, and makes validating the config more complex) 
    # The nonempty list of supported systems.
# systems = [ "x86_64-linux" "x86_64-darwin" ];
  3. (hides what the default value is) 
    # The nonempty list of supported systems.
# Available values are: x86_64-linux, x86_64-darwin
# systems =

Would be good to have a worked-out example of using a flake using flakeOutputsPrefix to transition

Notes.

Can we default repoRoot to inputs.self?

Using inputs.self leads to a nasty infinite recursion bug. Eventually it'll be fixed and we can rid of repoRoot.

Why do we have our own list of allowable compilers? Can't we just use h.nix's and have haskellCompilers be a subset of that?

haskellCompilers already is a subset of h.nix's. It's a small subset, because each compiler needs to be "tested" i.e. make sure that its tools are availalble, cached, and working, and set to the correct version (stylish-haskell, hlint, fourmolu, cabal, cabal-fmt, hls). Eventually the haskellCompilers set will expand. For now it contains the two compilers that are used by the SC repos.

Ideally we'd have settings of different subcomponents (here's how you define the haskell project info, here's how you define dev shell info, here's how you define CI info, here's how you define readthedocs info) completely separate, but at least it would be good to have them grouped together clearly in the docs.

Noted.

haskellCrossSystem: Why only one host system, why not just a per-system thing? Perhaps with an enableCross :: System -> Bool?

Because one host system is what is needed by those repos that use it. enableCross would be overkill (as of now)

shellWelcomeMessage/shellPrompt: Would be good to link to generic docs for PS1, ANSI escape codes

Noted.

shellModule: What is a "devShell module"?

Obsolete term. This will be named shellFile or simply shell.

shellModule: Would be good to clarify that nothing in shell module means nothing added to the Haskell shell not that you get no shell at all

Noted.

Hydra: Maybe we should define our own simple CI interface that we can compile down to hydra or GHA or whatever? Since we don't have a stable CI solution across the teams yet. I've started some work in this direction if you're interested.

I think we should put that off until someone actually decides to transition to GHA or use something else. All SC repos I've seen so far use Hydra.

Would be good to explain why you don't want profiled builds in CI

Noted.

Would be good to explain why you would want to add something to the CI blacklist

Noted.

readTheDocsExtraHaddockPackages: Would be good if by default this could be "all of the locally declared packages in the cabal.project", and if we exported a function to start there and let the user add more.

Noted.

Is it possible end users may want extra outputs that aren't per-system?

It's possible, just I haven't witnessed it yet in the repos I've worked on, so I did not provision for it.

template/nix/haskell-project.nix

The type of this file is pretty complex and includes details we shouldn't need. Why does the haskell.nix definition take the entire flakeopts?

This file can be simplified. I can get rid of flakeopts altogether.

Ideally this would be declarative (or declarative with the option to expand to something more involved as needed).

We had discussed this, and we had agreed that we should leave this for last or not do it at all, because creating a declarative layer on top of h.nix may end up being unachievable or not very useful, given the many edge-cases specific to each repository.

Should document that this is expected to return "a haskell.nix project value"

Noted.

The double negative of "systemized" is confusing here

Noted.

Why shouldn't we use inputs.nixpkgs?

Because we create an overlayed version of nixpkgs using h.nix (see src/bootstrap/pkgs.nix). Users should use that because it's configured and overlayed properly: using two nixpkgs is asking for trouble. This explanation will be added to the docs

If the user has to call cabalProject', what are we adding? Shouldn't the invocation be in lib with upstream-controlled defaults/interpretation/etc.?

Yes we can simplify this further by moving the invocation out of the template and making the template simpler.

shlevy commented 1 year ago

I completely agree about the need for quick iteration and feedback, and that we don't know up front what complexity and functionality will be needed. I will be better about distinguishing "directional" feedback (it would be good over time if we moved in this direction) from "immediate" feedback (I think this should be changed now). I think what we have here is basically good enough to call "1.0", as long as we clearly convey that the interface is fluid and that we intend to improve along various dimensions of clarity, compositionality, etc.

Also there were a few places where it seems I didn't explain well enough what I was suggesting, I will endeavor to provide more illustrative examples moving forward.

Directional comment: For composition, there are alternatives beyond "a single all-encompassing interface" and "fully generic sequential stacking" like the example interface you gave. Indeed, your "filesystem-based all the way" idea illustrates this: mkFlake needn't take a list of individual knobs meeting some common abstract interface without being able to peek inside, mkFlake can take (whether as an argument, or pulled from the fs) a declarative description of a haskell project, of a CI config, etc., and construct its results from those knowing the details of what they are. I'm not sure I'm explaining this very well, let me know if this is unclear and I can try again or maybe we can discuss in a team meeting. I'm much more concerned about interface composibility than internal composibility, in part because I think if we're thoughtful about that interface internally then the internal codebase will necessarily respect the boundaries and be less brittle.

I think fs-based is good, but FWIW even nested structure within a single attrset would be an improvement, not "cheating".

As I've mentioned, I like this fs-based approach. One possible change if the nix folder starts getting hairy is to put these files closer to the code they're configuring (e.g. h.nix stuff next to cabal.project, rtdocs stuff in docs folder).

The fs-based h.nix stuff without needing to call cabalProject' directly is what I was suggesting with a declarative interface, not that we create our own declarative layer on top of h.nix.

Directional comment: I was not suggesting removing templates, I agree they're a very good idea. I just think we should also have standalone docs that remain up-to-date and do not serve double-duty of explaining and actually providing a working config.

Directional: I prefer omitted fields (or files, in the fs-based approach) over templated explicit defaults. Semantically, "I've explicitly set this field to its default value" and "I've left the field defaulted" are different; The latter means that the implementation is free to make the omitted value depend on other data passed to it, and also allows it to change the meaning over time (we found a better default, or we added a new component that you get for free without a new file). Also, this helps with the separation of what we're responsible for vs what the user is responsible for: They can keep their codebase limited to lines that they understand and have chosen, and anything else is coming from us.

Eventually it'll be fixed and we can rid of repoRoot.

Can we add documentation to this effect? Ideally with a link to a bug ticket (in iogx if we don't know what is responsible for the bug yet, or upstream if we do)

each compiler needs to be "tested" i.e. make sure that its tools are availalble, cached, and working, and set to the correct version

Got it. Two thoughts:

Everything else looks great, thank you!

zeme-wana commented 1 year ago

In light of your feedback these will be the next steps:

  1. Transition to full fs-based interface
  2. Replace flakeopts with smaller attrsets in various ./nix/* interface files
  3. Improve documentation throughout, addressing all points raised in this issue, including a new README.md
  4. Keep using templates
  5. Fix repoRoot = inputs.self bug
  6. Consider replacing templated explicit defaults with omitted fields and files (I'm not 100% sold on this yet)

@shlevy You can close this issue if you agree with this plan

shlevy commented 1 year ago

Yes, sounds good!