Skip to content
This repository was 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
Open

Native loading strategy to support TRUFFLE_NATIVE_SOLC_PATH #6007

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5708c3f
Native loading strategy to support TRUFFLE_NATIVE_SOLC_PATH
hellwolf Apr 25, 2023
3e68670
Update Native.ts
hellwolf Apr 27, 2023
530e64c
Merge branch 'develop' into patch-3
hellwolf Apr 27, 2023
48a9981
Hook up Native Path
cds-amal Apr 30, 2023
b8bd147
Update packages/config/src/configDefaults.ts
cds-amal May 13, 2023
63ce524
RangeUtils should create Native with nativePath
cds-amal May 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/compile-solidity/src/compilerSupplier/loadingStrategies/Native.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Contributor

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
Contributor

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
Contributor

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.

const maxBuffer = 1024 * 1024 * 10;

try {
Expand Down