Build workspace packages in top-sorted order #1018
Conversation
|
The three cases I test for are similar to but not quite those described in the #988. Unless we publish a package that intentionally has a warning in it, it will not be possible to test for those cases specifically. So, the best we can do is test against local workspace packages that satisfy that criteria. |
Future changes will cause Fetch, Repl, and Build to have such a cycle
- Fetch.run returns back the same value as it did before (i.e. `dependencies`) but now under the `dependencies` field name. Under the `packageDependencies` field name, it returns a map that can be used to look up all the dependencies for a given workspace package. This is used in Build.run to get all the dependencies for a given package. However, to produce the `packageDependencies` value, we have to call `getTransitiveDeps` multiple times and don't get to share the work done in previous calls. So, if 5 packages all depend on `halogen`, we're recomputing `halogen`'s transitive dependencies 5 times. - Fetch.run's internal implementation was refactored a bit to prevent the impossible case where `workspace.selected` is `Just` but calling what was `getConfigPath` was `Nothing`.
|
Tasks remaining:
|
|
@f-f For this code spago/src/Spago/Purs/Graph.purs Lines 54 to 56 in 32614da could you clarify what the mappings are? |
|
The comment about the type is meant to clarify just that, but I guess it can be cryptic - the idea of the mapping is: for every Package that we depend on, we note which Module we are depending on, and for each of them we note from which Module we are importing it. |
How does this work when a non-js backend is used? There's already a "Backend build succeeded" message. |
|
Remaining tasks:
|
Ah yes, indeed - when building multiple packages with a backend we'd first build with purs, then build with the backend, then repeat. Alright, let's leave this out of this PR and just open an issue once we merge |
There was a problem hiding this comment.
This is great now - thanks @JordanMartinez!
Let's add the last test for ensureRanges, then we can merge in
|
In latest commits, I...
I won't merge this despite the approval in case you have more feedback. But if it looks good, then feel free to merge it. |
I've tracked our conversation about this here and added it to the original issue: #988 (comment) |
Description of the change
First part of #988.
Tests added cover these cases:
flowchart TD subgraph "Case 1" A ---> Dep0["effect, console, prelude"] B ---> Dep0 end subgraph "Case 2" A2 ---> effect2 A2 ---> console2 A2 ---> prelude2 A2 ---> Shared2 B2 ---> effect2 B2 ---> console2 B2 ---> prelude2 B2 ---> Shared2 Shared2 ---> prelude2 end subgraph "Case 3" A3 ---> effect3 A3 ---> console3 A3 ---> prelude3 A3 ---> B3 A3 ---> C3 B3 ---> effect3 B3 ---> console3 B3 ---> prelude3 B3 ---> C3 C3 ---> prelude3 end subgraph "Case 4 (duplicate module)" A4 ---> effect4 A4 ---> console4 A4 ---> prelude4 B4 ---> effect4 B4 ---> console4 B4 ---> prelude4 C4 ---> prelude4 endChecklist:
READMEP.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊