Add solcjs-get #227

chevdor · 2018-06-19T20:49:26Z

$ ./solcjs-get --help
Usage: [options| version]

Options:
  --version   Show version number                                      [boolean]
  --list      List all the versions
  --all       Download all the versions. Additionnaly, use --nightly to get the
              nightly builds.
  --nightly   Combined with --all, it adds the nightly to the list
  --releases  Get all the release builds
  --clean     Delete all the compiler in ./bin
  --help      Show help                                                [boolean]

coveralls · 2018-06-19T21:03:46Z

Coverage remained the same at 85.828% when pulling 2fa53cd on chevdor:solcjs-get into e5fab6f on ethereum:master.

chevdor · 2018-06-21T12:57:55Z

@axic you were asking for this one

axic

This shares a lot of code with downloadCurrentVersion.js. The common code should be pulled into helpers.

Alternatively, this should just replace downloadCurrentVersion.js.

chevdor · 2019-01-17T17:08:35Z

It replaces downloadCurrentVersion.js

axic · 2019-01-18T00:31:10Z

It replaces downloadCurrentVersion.js

In the PR it doesn't do that yet. I can look into fixing it if you don't have time (which I understand, given how long it took to get into reviewing the PR).

chevdor · 2019-01-18T10:45:28Z

I admit I lost the history of this one...
I initially made a big PR (very likely too big). Upon your request I then shrunk it a bit so parts may have been left out intentionally. Feel free to fix it as you wish but keep an eye on the other PRs as your fixes may be already done elsewhere.

chevdor · 2021-07-08T08:43:17Z

Hello, coud this PR be reviewed and either merged or closed ?

chriseth · 2021-07-08T10:31:26Z

Can you explain the benefit over downloadCurrentVersion.js?

chevdor · 2021-07-08T10:52:42Z

Can you explain the benefit over downloadCurrentVersion.js?

Back in 2018 I surely could have 😄
From what I recall, instead of downloading ONLY the current version, it allowed the user downloading the version of their choice.

cameel · 2021-07-09T17:49:16Z

I think this looks useful. It's at least an improvement over the original script.

I rebased it on the latest master and added some small tweaks (typos, parameter descriptions, style). I also modified so that the new script actually replaces downloadCurrentVersion.js.

There are some things that I think get unnecessarily removed. I'll point them out in review.

BTW, the diff is much better if you look at the individual commits. In the combined diff github does not detect that the file was actually just renamed.

cameel · 2021-07-09T17:56:59Z

solcjs-get

-        var hash = '0x' + keccak256(fs.readFileSync(outputName, { encoding: 'binary' }));
-        if (expectedHash !== hash) {
-          console.log('Hash mismatch: ' + expectedHash + ' vs ' + hash);
-          process.exit(1);
-        }


The new script removes the code responsible for verifying checksums of downloaded binaries. I don't think it's a good thing. I'd keep that code.

cameel · 2021-07-09T17:57:34Z

solcjs-get

-  var expectedFile = list.builds.filter(function (entry) { return entry.path === releaseFileName; })[0];
-  if (!expectedFile) {
-    console.log('Version list is invalid or corrupted?');
-    process.exit(1);
-  }


This check is gone too in the new version.

cameel · 2021-07-09T17:58:17Z

solcjs-get

-  process.on('SIGINT', function () {
-    console.log('Interrupted, removing file.');
-    fs.unlinkSync(outputName);
-    process.exit(1);
-  });


This used to remove a partially downloaded file. I'd keep it.

cameel · 2021-07-09T17:58:46Z

solcjs-get

 var MemoryStream = require('memorystream');
-var keccak256 = require('js-sha3').keccak256;
+var path = require('path');
+var compilerDir = 'bin';


I'd add an option to specify this directory (bin could still be the default though).

cameel · 2021-07-09T18:01:16Z

solcjs-get

+  .option('list', {
+    describe: 'List all the versions',
+    type: 'bool'
+  })
+  .option('all', {
+    describe: 'Download all the versions. Additionnaly, use --nightly to get the nightly builds.',
+    type: 'bool'
+  })
+  .option('nightly', {
+    describe: 'Combined with --all, it adds the nightly to the list',
+    type: 'bool'
+  })
+  .option('releases', {
+    describe: 'Get all the release builds',
+    type: 'bool'
+  })
+  .option('clean', {
+    describe: 'Delete all the compiler in ./bin',
+    type: 'bool'
+  })


These options all seem to be mutually exclusive but the script does not print any error if you use more than one. This was pretty confusing when I tried running it for the first time. Using more than one should not be allowed unless it actually does something.

BTW, I think that combining --list with --all/--nightly/--releases might actually be useful. I'd even make --list print only releases by default because the whole list is pretty long.

cameel · 2021-07-09T18:04:47Z

solcjs-get

+      if (!requestedVersion){
+        fs.copy(file, 'soljson.js'); // for backward compatibility
+      }


This should really be a symlink (at least if the FS supports it).

Actually, this does not seem like something just for backwards compatibility. Can solc-js even use the compiler when it's inside bin/? I think that an option to relink it to another version might be useful because that's basically how the select the compiler version now.

cameel · 2021-07-09T18:08:33Z

solcjs-get

+  console.log('Removing all local compilers in ' + compilerDir + ' ...');
+  fs.remove(compilerDir, function (err) {
+    if (err) return console.error(err);
+
+    console.log('Success! Cleaned ' + compilerDir);
+    process.exit(0);
+  });


Not sure if it's a good idea to remove the whole dir. As a user I think I'd expect this to be more intelligent and only remove the binaries (otherwise why even provide an option for something that can be easily done with rm?). I think it would be safer to only remove files matching the valid name regex.

cameel · 2021-07-09T18:11:21Z

solcjs-get

+    list = JSON.parse(list).builds;
+    for (var i = list.length - 1; i >= 0; i--) {
+      var target = list[i].path;
+      if (target.indexOf('nightly') > 0 && !argv.nightly) continue;


The list contains a releases array. I think that checking if the file name is not in it would be a more future-proof check for a nightly.

cameel · 2021-07-09T18:24:54Z

solcjs-get

+var compilerDir = 'bin';
+
+var yargs = require('yargs')
+  .usage('Usage: [options| version]')


Is this actually supposed to work with a version or only with the whole file name?
Something like ./solcjs-get 0.8.6 or ./solcjs-get v0.8.6 does not not say that the file does not exist. The script tries to download it:

Retrieving available version list... Requested version: v0.8.6 Downloading version v0.8.6 Error downloading file: 404

chevdor · 2021-07-14T07:19:02Z

Closing because I don't really have a use for this PR anymore and it is so long ago, I don't even remember the details...

chevdor mentioned this pull request Jun 19, 2018

Add support for options #63

Closed

axic suggested changes Jan 17, 2019

View reviewed changes

axic force-pushed the solcjs-get branch from d1a28b2 to ba494c7 Compare July 4, 2019 08:42

chevdor requested a review from axic July 8, 2021 08:43

cameel and others added 2 commits July 9, 2021 19:39

Rename downloadCurrentVersion.js to solcjs-get

93cd94a

Add solcjs-get

9e3de5e

cameel force-pushed the solcjs-get branch from ba494c7 to cefbdc6 Compare July 9, 2021 17:46

cameel reviewed Jul 9, 2021

View reviewed changes

cameel added 4 commits July 9, 2021 20:18

fixup! Add solcjs-get

9a24ab5

fixup! Add solcjs-get

d9c58be

fixup! Add solcjs-get

7f987f5

fixup! Add solcjs-get

2fa53cd

cameel force-pushed the solcjs-get branch from e9191fe to 2fa53cd Compare July 9, 2021 18:21

cameel reviewed Jul 9, 2021

View reviewed changes

chevdor closed this Jul 14, 2021

Add solcjs-get #227

Add solcjs-get #227

Uh oh!

Conversation

chevdor commented Jun 19, 2018

Uh oh!

coveralls commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chevdor commented Jun 21, 2018

Uh oh!

axic left a comment

Choose a reason for hiding this comment

Uh oh!

chevdor commented Jan 17, 2019

Uh oh!

axic commented Jan 18, 2019

Uh oh!

chevdor commented Jan 18, 2019

Uh oh!

chevdor commented Jul 8, 2021

Uh oh!

chriseth commented Jul 8, 2021

Uh oh!

chevdor commented Jul 8, 2021

Uh oh!

cameel commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chevdor commented Jul 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jun 19, 2018 •

edited

Loading

cameel commented Jul 9, 2021 •

edited

Loading

cameel Jul 9, 2021 •

edited

Loading