Insert reportedVersion in list.json #21

axic · 2018-06-27T11:26:25Z

fanatid · 2018-06-27T11:33:38Z

@axic try with execSync, example:

const cp = require('child_process')
console.log(cp.execSync(`node -e "console.log(require('./bin/soljson-v0.3.1-nightly.2016.4.7+commit.54bc2a').cwrap('version', 'string', [])())"`).toString())

axic · 2018-06-27T11:42:02Z

Thanks! That was my idea for workaround too, but would be nice to figure out what is leaking memory.

axic · 2018-06-27T12:29:28Z

@fanatid @chriseth can you review?

package.json

fanatid · 2018-06-27T17:11:12Z

29min in Travis -- it's crazy. Also, do you need to run npm run update locally every time on adding new binary?

axic · 2018-06-27T17:21:31Z

Also, do you need to run npm run update locally every time on adding new binary?

Yeah, but it is only done on releases and for every nightly.

fanatid · 2018-06-27T17:29:08Z

If Travis should only verify that files is OK, we can speed-up it with matrix build. But if you are ok with 30min on CI, changes is not worth it.
Regarding running it locally, if you need I'd happy push PR with code for multi-process update. Let me know.

axic · 2018-06-29T15:51:25Z

The list must be updated before merging.

axic · 2020-05-17T14:44:54Z

update

+    // NOTE: should be using this, but it leaks memory
+    // return solc(require(path.join(__dirname, '/bin', file))).version()
+    const filename = path.join(__dirname, '/bin', file)
+    return cp.execSync(`/usr/bin/env node -e "console.log(require('${filename}').cwrap('version', 'string', [])())"`).toString().trim()


An added side-effect of this is it would fail if the binary can't be loaded or is missing a the version function (e.g. incompatible).

This would be a problem because we have one executable ( soljson-v0.4.1-nightly.2016.9.9+commit.79867f49.js) that crashes when you run it.

Lol what? Well, there can be an exception.

Well, we could, but I'm still not sure if running the executables while generating the list is such a good idea. It's slow enough already and we still have this weird problem with nodejs not exiting immediately.

BTW, here's the error from that executable:

TypeError: Invalid Version: 0.4.1-nightly.2016.09.09+commit.79867f49.Emscripten.clang at new SemVer (/home/cameel/data/working-set/projects/sol/solc-js/node_modules/semver/semver.js:323:11) at compare (/home/cameel/data/working-set/projects/sol/solc-js/node_modules/semver/semver.js:614:10) at Function.gt (/home/cameel/data/working-set/projects/sol/solc-js/node_modules/semver/semver.js:643:10) at setupMethods (/home/cameel/data/working-set/projects/sol/solc-js/wrapper.js:20:27) at Object.<anonymous> (/home/cameel/data/working-set/projects/sol/solc-js/index.js:3:18) at Module._compile (node:internal/modules/cjs/loader:1101:14) at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10) at Module.load (node:internal/modules/cjs/loader:981:32) at Function.Module._load (node:internal/modules/cjs/loader:822:12) at Module.require (node:internal/modules/cjs/loader:1005:19)

Looking at it now, I see that it's actually a problem in solc-js, not the executable so it should be fixable. The string it reports is probably just not valid semver. It seems to be the 09 in version. Later executables have no zeros and don't crash.

ethereum/solc-js#562

solc-js or emscripten causes some kind of memory leak and node.js runs out of memory

chriseth · 2020-05-18T09:28:46Z

Needs rebase.

axic · 2021-03-12T19:47:31Z

@cameel what do you think about this? Since you have been rebuilding binaries. The goal of this was to include the version hardcoded in the binary into the JSON list. Currently (or at the time of this PR) the JSON only contained the normalised version, but certain old compilers do not follow that. Here's a list of discrepancies: https://github.com/ethereum/solc-js/blob/master/translate.js#L3

cameel · 2021-03-12T20:49:55Z

I'm not sure if it's actually useful for users but it would definitely be helpful for us to see these versions somewhere just as a sanity check. Putting them on the list does make them and any changes pretty visible.

At times I did need that info myself and I had to gather it manually. I think that seeing them would also make us spot some problems much earlier: #64, ethereum/solidity#7512, ethereum/solidity#10183.

cameel · 2021-03-12T21:03:29Z

Here's a list of discrepancies: https://github.com/ethereum/solc-js/blob/master/translate.js#L3

The list is bigger :) Versions listed there look like they're just examples of each format. Actually all emscripten binaries below 0.4.0 use non-standard version format. And 0.4.0 and 0.4.2 also were built with uncommitted changes so their version strings contain .mod. I checked them all at some point - here's the full list of all version strings for emscripten releases and nightlies up to 0.7.3: #64 (comment)

axic · 2021-03-12T21:08:22Z

True, that list was the cases which need parsing. And ones with .mod still parse with semver, so they never showed up as issues. However you mentioning .mod makes me believe this would be actually a very useful feature for sanity check.

cameel · 2021-03-12T21:05:07Z

update

@@ -7,6 +7,7 @@ const path = require('path')
 const semver = require('semver')
 const ethUtil = require('ethereumjs-util')
 const swarmhash = require('swarmhash')
+const cp = require('child_process')


This is not used that often so I'd use full name for readability:

Suggested change

const cp = require('child_process')

const child_process = require('child_process')

cp reads like the command-line copying utility to me :)

very simple.😉

cameel · 2021-03-12T21:24:21Z

update

+      // NOTE: should be using this, but it leaks memory
+      // return solc(require(path.join(__dirname, dir, file))).version()
+      const filename = path.join(__dirname, dir, file)
+      return cp.execSync(`/usr/bin/env node -e "var solc = require('${filename}'); console.log(solc.cwrap(('_solidity_version' in solc) ? 'solidity_version' : 'version', 'string', [])())"`).toString().trim()


It looks like it's going to be very slow. When I was rewriting bytecode comparison scripts and I tried doing something like this, rerunning node for each compilation was very slow (ethereum/solidity#10165 (comment)). A loop in JS was one or two orders of magnitude faster.

Is it actually a leak or is it just that update itself is pretty bad about controlling its memory usage (ethereum/solidity#9956)? Because for the bytecode comparison I run prepare_report.js which compiles around 5k test cases in a loop, twice and it does not run out of memory in the t-bytecode-compare.yml action.

Nodejs is bad at controlling memory. We set the loaded emscripten modules to empty reference, but it still won't be GC'd in time somehow. Perhaps it is not a problem anymore with the wasm binaries?

ok, right. It's about loading the compiler into memory, not running it. Bytecode comparison loads only one version.

Perhaps it is not a problem anymore with the wasm binaries?

We still need to run it on all those old asm.js nightlies so I think this does not help us even if it's fixed for wasm. Unless we just compute versions for asm.js once and cache them.

The script now has an option that makes it not recalculate hashes. It could have a similar one for this. The downside would be that if you replace the binary, CI won't update the versions so it would be safer to somehow combine it with detecting which binaries changed in the PR (which adds a bit of complexity to the whole setup).

cameel · 2021-03-12T21:26:08Z

update

        const fileContent = readFile(pars.path)
+        pars.reportedVersion = readBuiltinVersion(pars.path)


One problem with adding it now is that the repo contains binaries for multiple platforms and you won't be able to get versions for all of them in one run. We'd need separate runs for different platforms and then we would have to combine gathered info.

cameel · 2021-03-12T21:29:44Z

Maybe we should have an extra check that newly added binaries do not contain .mod in their version string? We could easily do that in t-bytecode-compare.yml which already has logic to detect newly added binaries in new PRs (except for Windows ones). We would just have to limit it to versions above 0.4.2.

axic · 2021-03-12T21:31:49Z

Maybe we should have an extra check that newly added binaries do not contain .mod in their version string?

I think that would make sense in any case.

cameel · 2021-03-12T21:42:30Z

update

        const fileContent = readFile(pars.path)
+        pars.reportedVersion = readBuiltinVersion(pars.path)


I think we need an option for skipping this when running locally, just like you can skip hash recalculation.

r0qs · 2022-12-01T19:18:48Z

@axic Is this still a thing? I didn't understand the motivation for having the reportVersion in list.json.

If still relevant, I can finish that PR and also add the flag to ignore it when running locally, as you suggested here: #21 (comment).

However, I don't think we can improve its performance in the way that it is (it took almost one and half hours on my machine, but the out-of-memory error does not happen anymore).
Maybe it would be better to improve the performance of the whole process, i.e., taking over PR #115 and only after that revisiting this one if it still relevant?

Kurdlove

best way for this.

axic force-pushed the reported-version branch from 5de8046 to 14cf777 Compare June 27, 2018 12:28

fanatid reviewed Jun 27, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

chriseth approved these changes Jun 29, 2018

View reviewed changes

axic force-pushed the reported-version branch from 14cf777 to 75d4816 Compare July 30, 2018 23:06

axic commented May 17, 2020

View reviewed changes

axic added 5 commits May 17, 2020 16:00

Add log for interactiveness

1928f10

Insert reportedVersion in list.json

0a021cf

Read version using subprocesses

e16b705

solc-js or emscripten causes some kind of memory leak and node.js runs out of memory

f

f4accf9

f

84161a8

axic force-pushed the reported-version branch from 75d4816 to 84161a8 Compare May 17, 2020 21:46

cameel reviewed Mar 12, 2021

View reviewed changes

cameel mentioned this pull request Nov 10, 2021

Older nightlies with leading zeros in version string crash soljson ethereum/solc-js#562

Open

JRICH00 approved these changes Jun 1, 2022

View reviewed changes

cameel requested a review from r0qs November 22, 2022 15:21

cameel mentioned this pull request Dec 15, 2022

Make it easier for users to get nightly builds ethereum/solidity#13802

Closed

6 tasks

Kurdlove reviewed Jun 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert reportedVersion in list.json #21

Insert reportedVersion in list.json #21

axic commented Jun 27, 2018

fanatid commented Jun 27, 2018

axic commented Jun 27, 2018

axic commented Jun 27, 2018

fanatid commented Jun 27, 2018

axic commented Jun 27, 2018

fanatid commented Jun 27, 2018

axic commented Jun 29, 2018

axic May 17, 2020

cameel Mar 12, 2021

axic Nov 10, 2021

cameel Nov 10, 2021

cameel Nov 10, 2021

chriseth commented May 18, 2020

axic commented Mar 12, 2021

cameel commented Mar 12, 2021 •

edited

Loading

cameel commented Mar 12, 2021

axic commented Mar 12, 2021

cameel Mar 12, 2021

Kurdlove Jun 9, 2024

cameel Mar 12, 2021

axic Mar 12, 2021

cameel Mar 12, 2021

cameel Mar 12, 2021

cameel commented Mar 12, 2021

axic commented Mar 12, 2021

cameel Mar 12, 2021

r0qs commented Dec 1, 2022

Kurdlove left a comment

	const cp = require('child_process')
	const child_process = require('child_process')

		const fileContent = readFile(pars.path)
		pars.reportedVersion = readBuiltinVersion(pars.path)

Insert reportedVersion in list.json #21

Are you sure you want to change the base?

Insert reportedVersion in list.json #21

Conversation

axic commented Jun 27, 2018

fanatid commented Jun 27, 2018

axic commented Jun 27, 2018

axic commented Jun 27, 2018

fanatid commented Jun 27, 2018

axic commented Jun 27, 2018

fanatid commented Jun 27, 2018

axic commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented May 18, 2020

axic commented Mar 12, 2021

cameel commented Mar 12, 2021 • edited Loading

cameel commented Mar 12, 2021

axic commented Mar 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Mar 12, 2021

axic commented Mar 12, 2021

Choose a reason for hiding this comment

r0qs commented Dec 1, 2022

Kurdlove left a comment

Choose a reason for hiding this comment

cameel commented Mar 12, 2021 •

edited

Loading