Use elm-solve-deps-wasm instead of elm-json solve #558

mpizenberg · 2021-12-28T14:40:11Z

Hey, I discussed this a little bit over the incremental elm discord, and this is what using elm-solve-deps-wasm inside node-test-runner would look like.

elm-solve-deps-wasm basically is a WebAssembly port of the dependency solver inside elm-test-rs. Advantages over using elm-json as a binary are the versatility of the solver that you can customize to your needs (can work with lamdera for example), and lightweight compared to downloading a binary. The wasm crate is 100kb compressed and 280kb total package uncompressed.

On the downsides, it's currently not much tested, has worse error messages than elm-json, and customizability implies a bit more code (cf dependency-provider-offline.js provided as starting point).

Let me know what you think of this.

mpizenberg · 2021-12-28T15:32:21Z

PS: it's expected that tests are failing since the starter dependency-provider-offline.js file only works if dependencies are already available offline. Tests are passing on my machine when I run npm test.

mpizenberg · 2021-12-29T22:52:56Z

To complement dependency-provider-offline.js I've implemented another dependency-provider.js that contains both the offline and online logic, and basically reproduces the full logic of the solver in elm-test-rs in JS. I think I got it right but JS is so painfully unforgiving that there are probably some bugs left.

I've left untouched the dependency-provider-offline.js because it's much easier to read first, to get a grasp of the API, and then read the more complete dependency-provider.js, which has additional online behavior and also caches.

Since I haven't figured out (yet) how to interact with async functions and wasm, I've made the wasm interfaces sync, and thus needed to have sync versions of file reading and http requests. This means I've added sync-request to the dependencies. If you know how to prevent that, let me know.

There are still a few console logs for now, that would eventually be removed or replaced by proper logging.

mpizenberg · 2021-12-30T11:35:19Z

I think I've figured out a way to use async functions with wasm and rust, but unfortunately, due to the contaminating nature of async, it requires changes all the way through elm-solve-deps to the pubgrub rust package. And there are other WIP in pubgrub that are more prioritized than async, so I wouldn't expect to be able to use elm-solve-deps-wasm in an async way for a few months.

It’s used to install Elm and elm-format for development of node-test-runner. I re-added elm-tooling to devDependencies (not dependencies) since it’s only used during development now.

lydell

I pushed some modifications directly to your branch, I hope that’s ok!

lib/DependencyProvider.js

mpizenberg · 2022-01-02T13:42:51Z

I haven't looked at the cache within solve.js There are multiple caches inside this implementation. OnlineCacheVersion is a hash map that corresponds to the Json answered by the package server at the address /packages. For efficiency this is also stored on disk in all home inside a pubgrub/ directory. Like so, when in online mode, we can start by reading this file on dis, count package versions, and and only ask the server the new packages at packages/since/count. There is a temporary cache just for memoization of the answer to the function listing existing versions for a given package. This is just to avoid having to read the disk again to list installed versions and merge with existing version according to the package website. (This merge of local + server answer is to have compatibility with lamdera). Finally, I also cache the JSON of each package when needing to read the dependencies of a given package. Both in a memory cache and on disk inside the pubgrub subdirectory inside the elm home.

…

On Sun, Jan 2, 2022, 14:02 Simon Lydell ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/DependencyProvider.js <#558 (comment)> : > +// Cache of existing versions according to the package website. +let onlineVersionsCache /*: Map<string, Array<string>> */ = new Map(); + +// Memoization cache to avoid doing the same work twice in listAvailableVersions. +// This is to be cleared before each call to solve_deps(). +const listVersionsMemoCache /*: Map<string, Array<string>> */ = new Map(); We already have a cache in Solve.js. Can you explain how this cache comes into play? — Reply to this email directly, view it on GitHub <#558 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWFOCIUAMYOXOAT6OZLL33UUBEGVANCNFSM5K4IIYQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

lydell · 2022-01-02T14:48:53Z

The way the cache in Solve.js works is this:

Try to read elm-stuff/generated-code/elm-community/elm-test/0.19.1-revision6/dependencies.SHA256.json from disk.
If it exists, use that (it contains the "dependencies" field value for the generated Elm application used to run the tests).
Otherwise compute that.

Reading that json file is very fast.

The cache key is a SHA256 hash of the user’s elm.json, so it is only invalidated if the user changes their elm.json which usually isn’t that often.

I know that elm-json stores things in ~/.elm, but I’m not sure how that affects things the times elm-json is actually invoked.

Given the above, do you think the extra caching you built is needed? If so, can you give an example scenario?

mpizenberg · 2022-01-02T15:32:09Z

Ah yes it's a completely different level of caching. And I think all levels are useful. Elm-json employs a similar caching strategy. It's just that the nature of a dependency solver may need to ask some of the things I'm caching multiple times.

…

On Sun, Jan 2, 2022, 15:49 Simon Lydell ***@***.***> wrote: The way the cache in Solve.js works is this: 1. Try to read elm-stuff/generated-code/elm-community/elm-test/0.19.1-revision6/dependencies.SHA256.json from disk. 2. If it exists, use that (it contains the "dependencies" field value for the generated Elm application used to run the tests). 3. Otherwise compute that. Reading that json file is very fast. The cache key is a SHA256 hash of the user’s elm.json, so it is only invalidated if the user changes their elm.json which usually isn’t that often. I know that elm-json stores things in ~/.elm, but I’m not sure how that affects things the times elm-json is actually invoked. Given the above, do you think the extra caching you built is needed? If so, can you give an example scenario? — Reply to this email directly, view it on GitHub <#558 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWFOCMCYMFDQC54CWFO4FDUUBQWBANCNFSM5K4IIYQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

lydell · 2022-01-02T18:21:03Z

So, what’s an example scenario where the extra caching leads to a faster experience?

mpizenberg · 2022-01-03T00:02:11Z

So, what’s an example scenario where the extra caching leads to a faster experience?

There are few scenarios I have in mind such as:

Create another project with the same dependencies than the current project

Let's say you use some project as a starter for another project. Change a few things like the package name, or the source location. But the dependencies stay unchanged. All files needed to solve the dependencies will already be available offline, so no need to make a request to the package server.

Add a dependency

With the caches, most of what is needed is already on disk. Most of the time, only few new requests will be needed instead of 1 + 1 per dependency. Sometimes even 1 request will be enough.

If the offline solver fails, there is one mandatory request done to https://.../all-packages/since/${count} where count is the number of package versions already known. Which are the ones we knew since last time we did a .../all-packages/since/${lastCount} request. So this requests will be quite fast as it just lists the last few published packages since last time we updated this. It will look something like follows.

// curl -L https://package.elm-lang.org/all-packages/since/12970 | jq
[
  "bChiquet/[email protected]",
  "newmana/[email protected]",
  "newmana/[email protected]"
]

Then only when needing to fetch the dependencies of a given package for the first time, inside fetchElmJson we need to make a call to the package server like https://package.elm-lang.org/packages/elm/random/1.0.0/elm.json. But sometimes, that package elm json was already downloaded for solving dependencies in another project so no request is needed at all.

harrysarson · 2022-01-04T10:38:38Z

I think I like the direction of this, using a library (even if it is wasm) is much neater than shelling out to a different program. It has the green light from me in principle.

lib/DependencyProvider.js

mpizenberg · 2022-01-04T23:15:32Z

what happens if a package is deleted and then a different package is

immediately uploaded? In normal conditions, we request at since/${count-1} such that we also have the latest package we know of in the answer from the server. As such, we can verify that no package was removed. Let's say the last package we know is A. If A is deleted, and another package B is added, we will find B where we expected A in the response and deduce that something not normal happened. So we reset known versions. If another package than A is deleted, and a new package C is added, it's the same. Since C is more recent than A, it's C that will be the package at the frontier and we will get C where we expected A in the response. In this situation, we also reset packages. In general, if any number of packages we knew are deleted, either there will be an empty response, or, if new packages were added, there will be the wrong package at position ${count} compared to what we were expecting, and in these situations, we reset known packages. If packages that we don't know yet are deleted before we update known packages, it has no impact on our logic. It will be as if they never existed. Other packages depending on those will just fail to resolve due to no version existing for that dependency.

…

On Tue, Jan 4, 2022, 21:11 Harry Sarson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/DependencyProvider.js <#558 (comment)> : > +// Update the cache with a request to the package server. +function updateCacheWithRequestSince( + cachePath /*: string */, + remotePackagesUrl /*: string */ +) /*: void */ { + // Count existing versions. + let versionsCount = 0; + for (const versions of onlineVersionsCache.values()) { + versionsCount += versions.length; + } + + // Complete cache with a remote call to the package server. + const remoteUrl = remotePackagesUrl + '/since/' + (versionsCount - 1); // -1 to check if no package was deleted. + const newVersions = JSON.parse(request('GET', remoteUrl).getBody('utf8')); + if (newVersions.length === 0) { + // Reload from scratch since it means at least one package was deleted from the registry. what happens if a package is deleted and then a different package is immediately uploaded? — Reply to this email directly, view it on GitHub <#558 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWFOCKFSJ7WOPCOQEMBQZTUUNH5RANCNFSM5K4IIYQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

avh4 · 2022-03-26T16:10:28Z

FYI, I have a large app I can test it on. (Feel free to ping or @ me directly if needed.)

Also, re node 12, imo I think it'd be fine to just drop it now, so long as there's a clear error message explaining that you need node 14+ if you try to run it on an earlier version (or does npm even have a way to mark the package as requiring a certain node version?) Anyone committed to using node 12 can just lock to the current elm-test version.

lydell · 2022-03-26T16:17:50Z

re node 12, imo I think it'd be fine to just drop it now, so long as there's a clear error message explaining that you need node 14+ if you try to run it on an earlier version (or does npm even have a way to mark the package as requiring a certain node version?)

Yes, package.json has the engines field: #465 (comment)

mpizenberg · 2022-03-26T16:40:12Z

I don't think there is much left to discuss since the discussion happened on the other PR by @harrysarson . Now I guess we only need a bit of testing from people with big code base.
After that, for Jeroen (and others) it might be nice to extract the JS overlay on top of the wasm package that we ended up with here so that there is a JS package with just a couple function ready to be used as-is. Otherwise everyone will end with a slightly different JS overlay on top of the wasm package. We might want it for flexibility but it's more update and fix maintainability burden if issues are raised.

mpizenberg · 2022-03-28T21:16:48Z

One argument to keep compatibility with node 12 is that debian bullseye, which is the current stable debian uses Node 12.22.

I have had issues raised about node 10 compatibility for elm-test-rs for the previous stable debian, so it might become an issue for elm-test too if we remove node 12 too early. Locking an elm-test version is an option, but it's possible they need to update for a security patch. I don't know, might be simpler to release a security patch for an older release.

I think all should work with a minimum version of 12.20 (for the sync http requests via worker) so if we can keep it the minimum version let's keep it that way.

harrysarson · 2022-03-29T08:56:17Z

agree about node 12. We should update the minimum version in package.json (currently it is 12.13 I think) and possibly also ensure that our CI checks node 12.20

mpizenberg · 2022-03-30T18:40:08Z

I added 12.20.0 to the list of node versions tested in the test.yml action. Not sure if that works since I can't trigger CI myself. Maybe there is also a way to extract that from the node engine property but I haven't looked at how if possible.

lydell · 2022-03-30T20:22:48Z

.github/workflows/test.yml

@@ -13,7 +13,7 @@ jobs:
    strategy:
      matrix:
        os: [ubuntu-latest, macOS-latest, windows-latest]
-        node-version: [12.x, 14.x, 16.x, 17.x]
+        node-version: [12.20.0, 12.x, 14.x, 16.x, 17.x]


Is it worth testing both 12.20.0 and latest 12.x, or would it be enough with just 12.20.0?

Felt like both the explicit minimum version, and the likely version are worth checking but 🤷

mpizenberg · 2022-03-30T20:52:55Z

Maybe there is also a way to extract that from the node engine property but I haven't looked at how if possible.

Most likely it's not a problem to have that duplicate because if we change the minimum version in the code it will fail on CI and we will change it there too.

avh4 · 2022-03-30T22:50:57Z

Trying this out on my work project(s) with npm install -g mpizenberg/node-test-runner#elm-solve-deps-wasm (in a docker image):

Everything seems to work when using npm 6.14.16!

But when using npm 8.5.5 (node 14.19.1), npm install -g mpizenberg/node-test-runner#elm-solve-deps-wasm fails with:

npm ERR! npm ERR! code 127
npm ERR! npm ERR! path /root/.npm/_cacache/tmp/git-cloneXJGWfj
npm ERR! npm ERR! command failed
npm ERR! npm ERR! command sh -c elm-tooling install
npm ERR! npm ERR! sh: 1: elm-tooling: not found
npm ERR!
npm ERR! npm ERR! A complete log of this run can be found in: ...

Seemingly when trying to run the prepare hook?

mpizenberg · 2022-03-30T23:53:29Z

@avh4 It seems that npm v6 does not run prepare for global installs, while npm v8 does.

Then I suppose the error stems from the fact that elm-tooling is in the dev dependencies, so I suppose it is not installed and not accessible for a global install when prepare is run after install. I'm not sure cause I don't do much JS and node.

avh4 · 2022-03-31T00:29:10Z

ah yeah, that analysis sounds correct then. I'm not seeing any npm hooks that would be appropriate for this (post-install script that is only for when dev dependencies are installed). Maybe the solution is to change the prepare script to be if which elm-tooling; then elm-tooling install; fi ?

mpizenberg · 2022-03-31T06:27:54Z

Maybe the solution is to change the prepare script to be if which elm-tooling; then elm-tooling install; fi ?

That would not be windows-compatible would it?

lydell · 2022-03-31T06:40:00Z

@avh4 You can try adding --ignore-scripts to your npm install command.

avh4 · 2022-03-31T09:55:32Z

Oh, npm scripts still don't get sh on Windows?

I'll see if --ignore-scripts works; that would be fine for me, since it's gonna be in a Dockerfile. (Also, if it weren't for building a docker image, I wouldn't be using global install anyway.)

lydell · 2022-03-31T10:59:38Z

But it’s still a good point of prepare. If npm 8 runs prepare on global installs, that’s bad. People are going to do that. So I should look into running elm-tooling for development in this repo some other way (and update the elm-tooling docs). However, it’s unrelated to this PR.

mpizenberg · 2022-04-05T09:33:58Z

So it seems we have the green light to merge, and the elm-tooling prepare stuff can be fixed later in another PR since it's already impacting the current version of node-test-runner.

Anything else you want to check @harrysarson ?

harrysarson · 2022-04-05T17:08:00Z

There are a couple of changes I might suggest (in a future PR) so let's give this 1 month or so on master before cutting the new release

harrysarson · 2022-04-05T17:09:31Z

@mpizenberg do you want to draft a commit message for the squashed commit?

mpizenberg · 2022-04-05T22:10:50Z

Use a custom dependency solver library instead of the elm-json binary.

This PR introduces a custom dependency solver, extracted from the solver in elm-test-rs, improving the compatibility with other elm-based platforms such as Lamdera.

The core algorithmic part of the solver, called "pubgrub", is rooted in the same logic than used in elm-json but implemented differently in the rust package pubgrub-rs.
That core algorithmic logic has been ported into a WebAssembly package called elm-solve-deps-wasm dedicated to the constraints of the elm package ecosystem and providing the following solve_deps wasm function where the two functions passed as arguments js_fetch_elm_json and js_list_available_versions are two functions injected via the JS call.

pub fn solve_deps(
    project_elm_json_str: &str,
    use_test: bool,
    // additional_constraints_str: &HashMap<String, Constraint>,
    additional_constraints_str: JsValue,
    // js_fetch_elm_json(pkg: &str, version: &str) -> String;
    js_fetch_elm_json: js_sys::Function,
    // js_list_available_versions(pkg: &str) -> Vec<String>;
    js_list_available_versions: js_sys::Function,
) -> Result<JsValue, JsValue> {

In this PR, the JS overlay implementing those two js_fetch_elm_json and js_list_available_versions is taken care of in the form of a DependencyProvider class providing two functions solveOffline and solveOnline. In the case of the offline solver, listing of existing versions and discovery of dependencies is exclusively performed from the packages installed inside the elm home. In the case of the online solver, some information may be retrieved from the package server.

// same interface for solveOnline
solveOffline( 
  elmJson /*: string */,
  useTest /*: boolean */,
  extra /*: { [string]: string } */
) /*: string */ { ... }

Remark: since requests for the online solver need to perform http requests in a synchronous manner, an approach based on a web worker thread is used, requiring a minimum node version of 12.20.0.

mpizenberg · 2022-04-05T22:13:29Z

Above is an attempt at a summary of this PR for a commit message. Feel free to change/remove/add anything.

harrysarson · 2022-04-06T07:47:44Z

Thanks!

Use elm-solve-deps-wasm instead of elm-json solve

8ee3259

harrysarson added status: discussion Next step is not crystal clear. Let's discuss. status: waiting for review Pull request needs a review labels Dec 28, 2021

Implement also an online dependency provider

36d7f05

lydell added 5 commits January 2, 2022 11:44

Restore elm-tooling for internal use

02a8ef6

It’s used to install Elm and elm-format for development of node-test-runner. I re-added elm-tooling to devDependencies (not dependencies) since it’s only used during development now.

Rename files to PascalCase for consistency with other files

44c1162

Pointless changes to match other files

37bd471

Type check DependencyProvider.js with Flow

2435cf4

Remove unused DependencyProviderOffline.js – I hope that’s correct

8db1160

lydell reviewed Jan 2, 2022

View reviewed changes

lib/DependencyProvider.js Outdated Show resolved Hide resolved

lib/DependencyProvider.js Outdated Show resolved Hide resolved

lib/DependencyProvider.js Show resolved Hide resolved

lydell added 2 commits January 2, 2022 13:35

Comment out console.log so the tests can run

2b8bab4

Use object shorthand for module.exports like other files

df6e9c9

lydell reviewed Jan 2, 2022

View reviewed changes

lib/DependencyProvider.js Outdated Show resolved Hide resolved

Avoid reading elm.json again

bfc3e44

lydell force-pushed the elm-solve-deps-wasm branch from 6adc8a4 to bfc3e44 Compare January 2, 2022 13:05

harrysarson reviewed Jan 4, 2022

View reviewed changes

lib/DependencyProvider.js Outdated Show resolved Hide resolved

Attempt (failing, hanging) at custom sync GET requests

de4200a

mpizenberg force-pushed the elm-solve-deps-wasm branch from ab0bc27 to de4200a Compare January 7, 2022 13:27

mpizenberg added 2 commits January 8, 2022 11:59

Fix new worker path

90797b0

Remove forgotten console logging

a4be62d

mpizenberg added 2 commits March 30, 2022 14:40

Merge branch 'master' into elm-solve-deps-wasm

a2e58b4

Set node >= 12.20 and mocha security upgrade

c1f27d5

mpizenberg force-pushed the elm-solve-deps-wasm branch from ebb823e to c1f27d5 Compare March 30, 2022 12:44

Also test minimum node version on CI

2c64301

lydell reviewed Mar 30, 2022

View reviewed changes

harrysarson merged commit fd52f04 into rtfeldman:master Apr 6, 2022

mpizenberg deleted the elm-solve-deps-wasm branch April 6, 2022 14:16

lydell mentioned this pull request May 11, 2022

Add a command to use master version of elm-explorations/test #592

Merged

lydell mentioned this pull request May 29, 2022

0.19.1-revision8 Changelog #560

Closed

5 tasks

jfmengels mentioned this pull request Aug 21, 2022

Use elm-solve-deps-wasm instead of elm-json jfmengels/node-elm-review#81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use elm-solve-deps-wasm instead of elm-json solve #558

Use elm-solve-deps-wasm instead of elm-json solve #558

mpizenberg commented Dec 28, 2021

mpizenberg commented Dec 28, 2021 •

edited

Loading

mpizenberg commented Dec 29, 2021 •

edited

Loading

mpizenberg commented Dec 30, 2021

lydell left a comment

mpizenberg commented Jan 2, 2022 via email

lydell commented Jan 2, 2022

mpizenberg commented Jan 2, 2022 via email

lydell commented Jan 2, 2022

mpizenberg commented Jan 3, 2022 •

edited

Loading

harrysarson commented Jan 4, 2022

mpizenberg commented Jan 4, 2022 via email

avh4 commented Mar 26, 2022 •

edited

Loading

lydell commented Mar 26, 2022

mpizenberg commented Mar 26, 2022

mpizenberg commented Mar 28, 2022

harrysarson commented Mar 29, 2022

mpizenberg commented Mar 30, 2022

lydell Mar 30, 2022

mpizenberg Mar 30, 2022

mpizenberg commented Mar 30, 2022

avh4 commented Mar 30, 2022 •

edited

Loading

mpizenberg commented Mar 30, 2022

avh4 commented Mar 31, 2022

mpizenberg commented Mar 31, 2022

lydell commented Mar 31, 2022 •

edited

Loading

avh4 commented Mar 31, 2022

lydell commented Mar 31, 2022 •

edited

Loading

mpizenberg commented Apr 5, 2022

harrysarson commented Apr 5, 2022

harrysarson commented Apr 5, 2022

mpizenberg commented Apr 5, 2022

mpizenberg commented Apr 5, 2022

harrysarson commented Apr 6, 2022

Use elm-solve-deps-wasm instead of elm-json solve #558

Use elm-solve-deps-wasm instead of elm-json solve #558

Conversation

mpizenberg commented Dec 28, 2021

mpizenberg commented Dec 28, 2021 • edited Loading

mpizenberg commented Dec 29, 2021 • edited Loading

mpizenberg commented Dec 30, 2021

lydell left a comment

Choose a reason for hiding this comment

mpizenberg commented Jan 2, 2022 via email

lydell commented Jan 2, 2022

mpizenberg commented Jan 2, 2022 via email

lydell commented Jan 2, 2022

mpizenberg commented Jan 3, 2022 • edited Loading

Create another project with the same dependencies than the current project

Add a dependency

harrysarson commented Jan 4, 2022

mpizenberg commented Jan 4, 2022 via email

avh4 commented Mar 26, 2022 • edited Loading

lydell commented Mar 26, 2022

mpizenberg commented Mar 26, 2022

mpizenberg commented Mar 28, 2022

harrysarson commented Mar 29, 2022

mpizenberg commented Mar 30, 2022

lydell Mar 30, 2022

Choose a reason for hiding this comment

mpizenberg Mar 30, 2022

Choose a reason for hiding this comment

mpizenberg commented Mar 30, 2022

avh4 commented Mar 30, 2022 • edited Loading

mpizenberg commented Mar 30, 2022

avh4 commented Mar 31, 2022

mpizenberg commented Mar 31, 2022

lydell commented Mar 31, 2022 • edited Loading

avh4 commented Mar 31, 2022

lydell commented Mar 31, 2022 • edited Loading

mpizenberg commented Apr 5, 2022

harrysarson commented Apr 5, 2022

harrysarson commented Apr 5, 2022

mpizenberg commented Apr 5, 2022

mpizenberg commented Apr 5, 2022

harrysarson commented Apr 6, 2022

mpizenberg commented Dec 28, 2021 •

edited

Loading

mpizenberg commented Dec 29, 2021 •

edited

Loading

mpizenberg commented Jan 3, 2022 •

edited

Loading

avh4 commented Mar 26, 2022 •

edited

Loading

avh4 commented Mar 30, 2022 •

edited

Loading

lydell commented Mar 31, 2022 •

edited

Loading

lydell commented Mar 31, 2022 •

edited

Loading