Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Native loading strategy to support TRUFFLE_NATIVE_SOLC_PATH #6007

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

hellwolf
Copy link

@hellwolf hellwolf commented Apr 25, 2023

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
Copy link
Author

This would also be benefiting from #6008

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -5,7 +5,7 @@ const { NoVersionError } = require("../errors");
export class Native {
load() {
const versionString = this.validateAndGetSolcVersion();
const command = "solc --standard-json";
const command = (process.env.TRUFFLE_NATIVE_SOLC_PATH || "solc") + " --standard-json";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ??.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged the suggestion but still using "||"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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

@hellwolf
Copy link
Author

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
Copy link
Member

cds-amal commented May 1, 2023

@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
Copy link
Author

hellwolf commented May 1, 2023

@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
Copy link

cameel commented May 7, 2023

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.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

packages/config/src/configDefaults.ts Outdated Show resolved Hide resolved
@hellwolf
Copy link
Author

@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.

@@ -16,7 +17,8 @@ export function resolveToRange(version?: string): string {
//if version was native or local, must determine what version that
//actually corresponds to
if (version === "native") {
return new Native().load().version();
const nativePath = TruffleConfig.default().compilers.solc.nativePath;
return new Native(nativePath).load().version();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@cds-amal cds-amal dismissed their stale review June 27, 2023 19:17

Removing change request to expedite approval

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -62,6 +62,7 @@ export const getInitialConfig = ({
},
compilers: {
solc: {
nativePath: `${process.env.TRUFFLE_NATIVE_SOLC_PATH || "solc"}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants