Native loading strategy to support TRUFFLE_NATIVE_SOLC_PATH by hellwolf · Pull Request #6007 · ConsenSys-archive/truffle

hellwolf · 2023-04-25T08:34:53Z

PR description

Related to #4490

Todo

Add tests to verify nativePath resolves correctly, including in rangeUtils
clean up @cds-amal's contribution.

Testing instructions

Truffle compile to support the usage of process environment TRUFFLE_NATIVE_SOLC_PATH when compilers.solc.version is "native".

Documentation

I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

Breaking changes and new features

I have added any needed breaking-change and new-feature labels for the appropriate packages.

hellwolf · 2023-04-26T12:58:12Z

This would also be benefiting from #6008

cds-amal

Thanks @hellwolf !

Apologies for the empty review summary. I accidentally submitted before going to sleep.

I think this logic belongs in @truffle/config where other such concerns are kept. @truffle/config uses a some meta-programming techniques to store and calculate default values. See here; for example, the following is an example of how the default could work, though there is a better name than nativePath 🤔 @gnidan? @eggplantzzz , @haltman-at ?

// ...
compilers: {
   solc: {
     nativePath: `{process.env.TRUFFLE_NATIVE_SOLC_PATH || "solc"}`
// ...

Edit: Ooops.

cds-amal · 2023-04-27T03:44:40Z

  load() {
    const versionString = this.validateAndGetSolcVersion();
-    const command = "solc --standard-json";
+    const command = (process.env.TRUFFLE_NATIVE_SOLC_PATH || "solc") + " --standard-json";


Suggested change

const command = (process.env.TRUFFLE_NATIVE_SOLC_PATH || "solc") + " --standard-json";

const command = `${process.env.TRUFFLE_NATIVE_SOLC_PATH ?? "solc"} --standard-json`;

This would read easier using template literals and the nullish coalescing operator

This is nitpicking, but if we're going to argue about || vs ??, I'd kind of prefer || because what if someone sets this variable to the empty string as a way of attempting to unset it, or something? Idk exactly what the interface is on process.env, maybe this is a non-concern, but to my mind unless there's some particular falsy value you're worried about causing problems, it makes sense ot use || instead of ??.

I think you are right @haltman-at , that value can sometimes actually be empty string.

I merged the suggestion but still using "||"

Hey @hellwolf , sorry about the confusion. I requested changes for an issue I didn't document. I was tired and thought I'd comeback to it this morning, only to realize I submitted it. 😞 I updated my review.

No worries.

But I didn't know "nativePath" was an option, I don't seem to be able to find the reference to that in code.

But I didn't know "nativePath" was an option, I don't seem to be able to find the reference to that in code.

nativePath doesn't currently exist. However, the location I shared, is where one could define this new configuration, and set its defaults depending on the ENV. Of course, the setting will have to be consumed in loadingStrategies/Native.ts, and I still think there's a better name than nativePath.

haltman-at · 2023-04-27T04:41:35Z

Btw, this isn't actually a breaking change, right? It's marked as one, but I don't see how it would be...

hellwolf · 2023-04-27T17:50:53Z

Btw, this isn't actually a breaking change, right? It's marked as one, but I don't see how it would be...

not breaking change.

cds-amal · 2023-05-01T21:53:57Z

@hellwolf , @cameel : If you were able to specify solc's fullpath for native executable as below, would you still need to override the command line?

  // ...
  compilers {
    solc: {
      // ...
      version: "native",
      nativePath: "/tmp/solc-linux-amd64-v0.8.10+commit.fc410830"
      // ...
    }
  }

hellwolf · 2023-05-01T22:24:58Z

@hellwolf , @cameel : If you were able to specify solc's fullpath for native executable as below, would you still need to override the command line?
  // ...
  compilers {
    solc: {
      // ...
      version: "native",
      nativePath: "/tmp/solc-linux-amd64-v0.8.10+commit.fc410830"
      // ...
    }
  }

That could be a nice option!

cameel · 2023-05-07T19:47:23Z

If you were able to specify solc's fullpath for native executable as below, would you still need to override the command line?

We wouldn't. That would work just as well as an env var for us.

cds-amal

@hellwolf, this is what I had in mind. @gnidan might have opinions on this, and of course we should add tests.

hellwolf · 2023-05-14T14:36:20Z

@hellwolf, this is what I had in mind. @gnidan might have opinions on this, and of course we should add tests.

thank you, happy to test it out and report here after this is make available in a future release.

haltman-at · 2023-06-27T17:44:27Z

  if (version === "native") {
-    return new Native().load().version();
+    const nativePath = TruffleConfig.default().compilers.solc.nativePath;
+    return new Native(nativePath).load().version();


Hm, this always uses the default path. Should this function take an optional second argument so it can be more accurate, and the uses of it updated as necessary/possible?

Removing change request to expedite approval

cds-amal

I'm ready to approve, but want to check if others are ok with the environment override.

cds-amal · 2023-06-27T19:27:33Z

    },
    compilers: {
      solc: {
+        nativePath: `${process.env.TRUFFLE_NATIVE_SOLC_PATH || "solc"}`,


I snuck this env override w/ my changes, and want to be sure @eggplantzzz and @haltman-at are ok with it.

Eech. I am quite wary of that... @gnidan?

Native loading strategy to support TRUFFLE_NATIVE_SOLC_PATH

5708c3f

hellwolf mentioned this pull request Apr 25, 2023

Setting a custom path for the native solc binary #4490

Open

d10r mentioned this pull request Apr 26, 2023

Use fuji as canary testnet, run in Nix shell superfluid-org/protocol-monorepo#1373

Merged

cds-amal added breaking-change: @truffle/compile-solidity new-feature: @truffle/compile-solidity labels Apr 27, 2023

cds-amal previously requested changes Apr 27, 2023

View reviewed changes

hellwolf added 2 commits April 27, 2023 20:51

Update Native.ts

3e68670

Merge branch 'develop' into patch-3

530e64c

gnidan removed the breaking-change: @truffle/compile-solidity label Apr 27, 2023

Hook up Native Path

48a9981

cds-amal reviewed May 12, 2023

View reviewed changes

Comment thread packages/config/src/configDefaults.ts Outdated

cds-amal added 2 commits May 12, 2023 21:36

Update packages/config/src/configDefaults.ts

b8bd147

RangeUtils should create Native with nativePath

63ce524

haltman-at reviewed Jun 27, 2023

View reviewed changes

cds-amal reviewed Jun 27, 2023

View reviewed changes

	const command = (process.env.TRUFFLE_NATIVE_SOLC_PATH \|\| "solc") + " --standard-json";
	const command = `${process.env.TRUFFLE_NATIVE_SOLC_PATH ?? "solc"} --standard-json`;

Conversation

hellwolf commented Apr 25, 2023 • edited by cds-amal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description

Testing instructions

Documentation

Breaking changes and new features

Uh oh!

hellwolf commented Apr 26, 2023

Uh oh!

cds-amal left a comment • 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!

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!

haltman-at commented Apr 27, 2023

Uh oh!

hellwolf commented Apr 27, 2023

Uh oh!

cds-amal commented May 1, 2023

Uh oh!

hellwolf commented May 1, 2023

Uh oh!

cameel commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cds-amal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hellwolf commented May 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cds-amal left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hellwolf commented Apr 25, 2023 •

edited by cds-amal

Loading

cds-amal left a comment •

edited

Loading

cameel commented May 7, 2023 •

edited

Loading