-
Notifications
You must be signed in to change notification settings - Fork 846
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
Begin using component-based builds #4745
Comments
I've given this a bit more thought, and given the number of changes already planned for Stack 2.0, and the goal of releasing relatively soon, I'd rather hold off on this until Stack 2.2. I'm going to leave this in the P1 milestone for the Stack 2.2 release. |
What's the progress on this? 2.2 is the next non-patch release right? Correct me if I'm wrong, but the linked issues all are reverse dependencies on this. Are there other issues/PRs that are working towards this? |
I believe there is currently no progress on this, unless someone is working on it that I'm not aware of. This would definitely be a great addition to get in, and I'd be happy to provide feedback/mentoring if someone is interested in tackling it. |
So as @qrilka stated he won't be able to work on this, and this is needed for backpack support (and likely needed for additional cabal3 features such as public sublibraries). I started to look at the code a bit, I feel like this change is very pervasive in the code base such as changing all mentions of Package in Build to Component ? Is there type creation involved or is every notion of cabal component already in there (with a few additions to NamedComponent ?) Also I'm not sure to what extent this is impacting the build plan ? Or is it only impacting the Task level ? Maybe a more precise description of what you mean by "component-based builds" would help @snoyberg ? tl;dr I'm interested in doing this, but I wouldn't mind a little help to assemble a "todo" list. |
So, as far as I understand, the main structural change is to switch -- extracted from Stack.Build.ConstructPlan
Task { taskProvides = PackageIdentifier
(packageName package)
(packageVersion package)
-- ... other stuff Into something with an additional This is however, (still, as far as I understand), not enough for a real component-based build; we should build all the transitive dependencies of a component with an appropriate NamedComponent specifier (e.g build lib or internal-lib xyz only if it's something with more than one option). SO quite a bit of change in the |
@theobat thank you for volunteering to step up on this one, we'd definitely appreciate the help! I think you generally speaking have the right idea. But I'm not the person with the most up-to-date information on this. @qrilka has more details, but is currently on vacation. When he's back, he should be able to provide more concrete feedback. |
Thanks @theobat for your interest in this ticket, finally I'm back from my vacation and hopefully could help a bit here. |
hi @qrilka, thanks for the answer. I'm getting into Setup.hs in the cabal doc/code. If you have any pointer to a good ressource for learning about it, please let me know. |
Unfortunately I'm not sure if there is a good resource describing the interface |
So as far as I understand, stack already use Setup.hs to build packages. The real question is how to use setup.hs for components (pretty obvious with hindsight, but I was not aware of the former...). Here's what I gathered:
I should probably propose an example for this (examples are great when you're the reader...) in cabal. So my idea of the "shortest path" to have component based builds:
Now this is a simple and ad-hoc addition, and I don't see why Stack.Build.ConstructPlan should be involved, but I'm certainly missing something. Besides, my goal is to set up proper foundations for supporting backpack, so I suppose I should check how backpack works at the runhaskell cli level... |
If I understand correctly you're planning just to make components explicit when they were set implicitly as "build complete package"/"build complete package with tests/benchmarks". I think this could help with some issues where components appear only as targets as it will not be able to use components as dependencies (which requires ConstructPlan changes) and thus e.g. cycle from #4891 (comment) won't be treated properly. As I'm not very familiar how Backpack works under the hood it's not clear if your "shortest path" proposal could be enough for Backpack. |
Fine, let's talk about the grand plan then. It seems that -- | A complete plan of what needs to be built and how to do it
data Plan = Plan
{ planTasks :: !(Map PackageName Task)
, planFinals :: !(Map PackageName Task)
-- ^ Final actions to be taken (test, benchmark, etc)
, planUnregisterLocal :: !(Map GhcPkgId (PackageIdentifier, Text))
-- ^ Text is reason we're unregistering, for display only
, planInstallExes :: !(Map Text InstallLocation)
-- ^ Executables that should be installed after successful building
}
deriving Show At the very least, planTasks should have the full component name + package name. Now it's the first problem there is no such type. Something like: data PackageAndComponentName = Plan
{ packageName :: !PackageName
, componentName :: !NamedComponent
}
deriving (Show, Eq)
-- with NamedComponent as:
-- | A single, fully resolved component of a package
data NamedComponent
= CLib
| CInternalLib !Text
| CExe !Text
| CTest !Text
| CBench !Text
deriving (Show, Eq, Ord) And then we could use that to address the planTasks, which to my understanding would probably solve the circular dependency problem. -- | A task to perform when building
data Task = Task
{ taskProvides :: !PackageIdentifier -- FIXME turn this into a function on taskType?
-- ^ the package/version to be built
, taskType :: !TaskType
-- ^ the task type, telling us how to build this
-- ... etc ...
}
-- task type:
-- | The type of a task, either building local code or something from the
-- package index (upstream)
data TaskType
= TTLocalMutable LocalPackage
| TTRemotePackage IsMutable Package PackageLocationImmutable
deriving Show
Nothinf currently points in the From this point, we mostly have to change There are a bunch of problems though since the Installed packages only suppport : data Installed
= Library PackageIdentifier GhcPkgId (Maybe (Either SPDX.License License))
| Executable PackageIdentifier
deriving (Show, Eq) Which is not what we want (we want the full power of installed component that is the constructor list provided by NamedComponent). Seems like a pretty drastic change everywhere on a hard to navigate codebase ( for various reasons ; Execute.hs is huge and not modular, TemplateHaskell and CPP usage prevent the excellent haskell language server from functionning properly). |
I don't see any particular comments to what you have written with an exception of 1 question: what are the "modules" you have highlighted as bold? Did you mean components maybe? |
Yes It's a mistake I fixed it, thanks. |
Sure, no problem |
Hello @theobat |
Hi @qrilka, I've been busy the past ffew weeks, but I did get a better understanding of the whole stack build function calls. What seems to matter is the 3 last bullet points in the following list:
Now my main concern is that, what I planned to do above only represent half of the equation (constructing the plan and executing it). I'm stuck on the source map changes needed for now. I don't really understand how to gather the components dependencies from stack.yaml itself (or the cabal file for that matter).
So maybe you can help me with that ? Otherwise it'll need a bit more digging in the whole SourceMap thing. PS: as a side note, why is template haskell so pervasive in the code base ? What's his purpose ? It prevents the haskell-language-server to function properly, so maybe we should do something about this, it definitely hampers any effort on the codebase. |
Actually I have given a part of a response to your question in my reply above - SourceMap as a base of implicit snapshots it not quite compatible with components thus needs to be changed. And I guess dependency tracking also needs to be updated. The current dependency information is mostly Cabal 1.0 - i.e. tracking only dependencies between packages (and package ~ library) with some extra hacks on top (e.g. supporting sublibraries to some extent).
|
A stratospherically high overviewThis is the set of actions that are required for having full blown component based builds.
Now the reality is much worse, because there are a bunch of datatypes which are still heavily based on old cabal versions such as Package in Package.hs. As stated in the snoyberg comment of the file, the whole mess between Package, LocalPackage etc should be simplified, which is probably the best first step to envision. The grand master planFor now the grand master plan, has 2 groups : the descriptive changes (1 and 2) and the actionnable changes (3 and 4). Descriptive changes should not change stack's behavior. actionnable ones should change stack behavior. It should roughly follow this:
The plan has to be refined a bit more to be sure this (significant) effort is the right one. Request for comments @qrilka :) |
@theobat I didn't get what you mean in your item ii when you say that "InstalledMap misses the installed components" and also "this is not affecting the InstallMap type". If you say that we'll only need actually installed components - unfortunately I didn't check what does Cabal (or probably ghc-pkg) allow to find out (or regsiter): at least previously the situation was that only libraries could be registered and properly checked for being already registered. |
Maybe I don't understand what InstalledMap is all about, my understanding is that, if something has been installed, we track this in some kind of SQLite database which is updated whenever we build something new. I thought the question of recording the set of installed components was completely "controlled" and didn't think it could be a ghc/cabal thing. type InstalledMap = Map PackageName (Map NamedComponent (InstallLocation, Installed)) The toInstallMap and getInstalled (just below, same file) functions could use better comments though so feel free to give a better description so that I can improve their haddocks.
With the Package datatype changes, dependencies will be tracked at the component level (old & default behavior, being the main lib component), which means the check for circular dependencies should support that, whereas today it's enforced using this: type ConstructPlanState = Map PackageName (Either ConstructPlanException AddDepRes) As above I think something like type ConstructPlanState = Map PackageName (Map NamedComponent (Either ConstructPlanException AddDepRes)) Will be required. But this means we should have a single task per component, while a bunch of things are common between components so I thought of something like: type ConstructPlanState = Map PackageName (Map NamedComponent (Maybe ConstructPlanException), Maybe AddDepRes) Which would enable a set of components to trigger a single task. It would probably trigger less changes to the task datatype. Maybe each step of the plan should be tracked in a dedicated issue, don't you think ? |
Ouch, it looks like I was not seeing the difference between |
Could you elaborate on that ?
On second thought I'm fine with keeping everything in this one, we can separate the PR's themselves, it should be enough. |
See loadDatabase - it loads packages from a |
So what I'm talking about is likely this: EDIT:
This is necessary for DumpPackage rework (and InstalledMap etc) |
This sounds like a good thing, and I really appreciate the work done on it. Were showstoppers found, or is the plan still viable ? |
hey @simonmichael thanks for your interest, I lack the time to implement the plan drawn here, but most of it is still valid as far as I know. |
Yes this should be closed, I'll rephrase what's still valid in another dedicated issue. |
Originally discussed in #834, we shelved work on this until Cabal added support. Cabal added support for component based builds long enough ago that we can drop support for older versions and fully embrace a component based system (see #4475). Not only will this help address some bugs, but will also open the door for Backpack support (see #2540).
@qrilka is planning on taking a look at this to estimate how long an implementation will take. This is currently planned for Stack 2.0, but may be deferred if it turns out this is more complex than it seems.
Additional issues which may be affected by this (incomplete list):
The text was updated successfully, but these errors were encountered: