Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: fix windows tests with .exe suffix #111

Merged
merged 2 commits into from
Jun 28, 2024
Merged

Conversation

antonbaliasnikov
Copy link
Contributor

@antonbaliasnikov antonbaliasnikov commented Jun 27, 2024

What ❔

Fixes Windows tests with optional .exe suffix.

Why ❔

On Windows, executables should always have .exe extension.

On the other hand, process::Command in Rust can work it out, and .exe can be skipped in the filename if the file has no other extension (or it looks like even a suffix we use with -). There are lots of weirdness internally explained in rust-lang/rust#37519.

As the result, the following initialization code:

# self.executable = "solc"
let mut command = std::process::Command::new(self.executable.as_str());

will work correctly if executable name is solc or solc.exe on Windows, but will not be able to find an executable (even if it's in PATH) if the executable name is solc-* (e.g. solc-0.8.26 cannot be found). Only solc-0.8.26.exe can be found correctly on Windows by Command.

There is more info in the Command docs:

Platform-specific behavior

Note on Windows: For executable files with the .exe extension, it can be omitted when specifying the program for this Command. However, if the file has a different extension, a filename including the extension needs to be provided, otherwise the file won't be found.

This PR optionally adds .exe suffix on Windows for tests, but does not change behavior on other platforms where we pass just solc which is fine for all cases.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via cargo fmt and checked with cargo clippy.

hedgar2017
hedgar2017 previously approved these changes Jun 27, 2024
@hedgar2017 hedgar2017 marked this pull request as ready for review June 27, 2024 20:01
@antonbaliasnikov antonbaliasnikov force-pushed the aba-fix-tests-windows branch 2 times, most recently from 6540d83 to ca9ca29 Compare June 28, 2024 09:39
Copy link
Collaborator

@hedgar2017 hedgar2017 left a comment

Choose a reason for hiding this comment

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

Thank you sir for the deep investigation!
Indeed the Rust std tries to be too smart here.

@antonbaliasnikov antonbaliasnikov merged commit fcd5609 into main Jun 28, 2024
16 checks passed
@antonbaliasnikov antonbaliasnikov deleted the aba-fix-tests-windows branch June 28, 2024 12:32
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