Skip to content

Fix solc_standard_json regression on Windows - #191

Merged
montyly merged 1 commit into
devfrom
dev-fixsolcstandardregression
Jun 23, 2021
Merged

montyly merged 1 commit into
devfrom
dev-fixsolcstandardregression

Conversation

@Xenomega

Copy link
Copy Markdown
Member

There's another regression with crytic-compile due to the change in this commit:

version=get_version(solc, dict()),

It invokes solc --version but passes a blank dict for the env. Doing so will cause the following error on Windows:

  File "c:\users\x\documents\github\crytic-compile\crytic_compile\platform\solc.py", line 309, in get_version
    with subprocess.Popen(
  File "C:\Python\Python39\lib\subprocess.py", line 947, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "C:\Python\Python39\lib\subprocess.py", line 1416, in _execute_child
    hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
OSError: [WinError 87] The parameter is incorrect

This PR instead changes the blank dictionary to None, which will use the existing os.environ. Although disjointed behavior exists between how every compilation platform handles optional env variables and the logic should likely be cleaned up as well.

This was not tested on Linux/macOS, although None is already used as env for other platform invocations and is none problematic so this should be fine.

…vironment variables were being overriden, making solc invocation fail.
@Xenomega
Xenomega requested a review from montyly June 23, 2021 05:42
@montyly
montyly changed the base branch from master to dev June 23, 2021 06:39
@montyly montyly mentioned this pull request Jun 23, 2021
@montyly
montyly merged commit c3fc19d into dev Jun 23, 2021
@montyly

montyly commented Jun 23, 2021

Copy link
Copy Markdown
Contributor

Nice catch @Xenomega, thanks!

@montyly
montyly deleted the dev-fixsolcstandardregression branch June 23, 2021 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants