Skip to content
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

Document an overview of builds #4299

Merged
merged 3 commits into from
Sep 12, 2018
Merged

Document an overview of builds #4299

merged 3 commits into from
Sep 12, 2018

Conversation

snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Sep 8, 2018

With all of the various refactorings at play, I wanted to document what
the end goal of this whole process would be, both to guide my own work,
an to get feedback from others (either to improve the process, or
clarify it so others can participate in the maintenance of this better).

Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

With all of the various refactorings at play, I wanted to document what
the end goal of this whole process would be, both to guide my own work,
an to get feedback from others (either to improve the process, or
clarify it so others can participate in the maintenance of this better).
@snoyberg
Copy link
Contributor Author

snoyberg commented Sep 8, 2018

@ezyang If you have any feedback on how this may affect Backpack, please let me know. As far as I know, the described procedure here should make it significantly easier to add Backpack (which is one of my explicit goals in this overhaul).

doc/build-overview.md Show resolved Hide resolved
Apply GHC options from the command line to all _project package
targets_. *FIXME* confirm that this is in fact the correct behavior.

## Apply general flags (CLI and config value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only CLI, not from config


## Apply general flags (CLI and config value)

*FIXME* figure out and document exactly which packages these will
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applies to any project package which has such a flag name available.

note is removed.

This is a work-in-progress document covering the build process used by Stack.
Stack. It was started following the Pantry rewrite work in Stack (likely to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated "Stack."

of _immutable_ packages. Only packages from immutable sources and which
depend exclusively on other immutable packages can be in this database.
*QUESTION* Would this better be called the _immutable database_ or _write only
database_?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer "write only" as it is closer to the way it operates.

* `packages`
* `extra-deps`
* `flags`
* `ghc-options`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a line specifying what's done if a stack.yaml file only has a subset of these, for completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add defaulting comments

* For all package name + component, ensure that the package is a
project package, and add that package + component to the set of
project targets.
* Ensure that no target has been specified multiple times.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will need an extra consistency step for internal libraries. Sometimes stack needs to use the mangled name (z-package-internallibname-z..), sometimes the package:internallibname one. But I think this will become obvious when doing the code changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to copy your comment in as a FIXME.

* Include all library dependencies for all enabled components
* Include all build tool dependencies for all enabled components
(using the fun backwards compat logic for `build-tools`)
* Apply the logic recursively to come up with a full build plan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note here: one awkward thing about formulating build plan construction this way is that you might reach package p via an edge that says "I only need the library", and then later in the recursive traversal you reach p again where it says, "Oh, I also need the executable." So if you want to implement it this way, you have to have a notion where you can take a package that is already in your plan, and increase the components that are enabled for it. If you can truly assume that all packages support per-component builds, then you can just memoize over components and there isn't any problem; this might help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some other approach to build plan construction which doesn't present this issue? I'm also not quite following your last sentence, can you clarify?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some other approach to build plan construction which doesn't present this issue?

Well, there are a few alternate approaches, but they have their own tradeoffs.

  1. You could say, recursive traversal operates over components, not packages. So you'll "visit" packages multiple times, but you'll only visit any given component once. The downside is that you'll end up having split up a package into multiple components to build, but some packages can't be built on a per-component basis (e.g., a package with a Custom setup) so you'll have to agglomerate them all back together before you actually do a build. (This is what I was alluding to in the last sentence.)

  2. You could process per-package, but say, "Actually, I'm going to assume that all components are requested." So you will never have to improve because you always greedily configured every component, so it must be available (or be impossible). The downside is that you spent a whole bunch of work figuring out a build plan to build everything, which is not actually what you want, so you're going to have to prune it down after the fact. You probably don't want to be recomputing this every time, so you had better cache the plan. (By the way, this is what cabal new-build does).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I think I'll leave the doc as-is for now, and update it during implementation. I have a feeling we'll end up closer to your (1).

*FIXME* There's some logic to deal with cyclic dependencies between
test suites and benchmarks, where a task can be broken up into
individual components versus be kept as a single task. Need to
document this better. Currently it's the "all in one" logic.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backpack would hook into this step: when you were configuring a component, you'd also be obligated to mix-in link it (assuming that the things that it referred to have recursively already been resolved).

Then, after everything is said and done, you can go over fully instantiated references to packages, and add extra jobs to the build graph to build the instantiated forms of those packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that we can only add these extra jobs to the build graph after we've performed the build, or are we able to create a complete build graph during plan construction? The former sounds fairly awkward.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're still able to create a complete build graph during plan construction. Actually, I suppose you can add the instantiated packages as you're initially building the build graph, but I think it's easier to do split it into two phases, just from a code complexity perspective.

@snoyberg snoyberg mentioned this pull request Sep 9, 2018
@snoyberg
Copy link
Contributor Author

Thanks for the feedback @mihaimaruseac and @ezyang. I'm going to merge this and start using it as a blueprint for further work.

@snoyberg snoyberg merged commit f1121dc into master Sep 12, 2018
@snoyberg snoyberg deleted the build-overview branch September 12, 2018 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants