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

Update the PATH environment variable to be able to run solidity tests… #2395

Merged

Conversation

philippecamacho
Copy link
Contributor

@philippecamacho philippecamacho commented Dec 12, 2024

I made this PR because I was not able to run the solidity tests using nix. The problem was that the diff-test executable was not in the PATH environment variable.

This PR:

Update the PATH environment variable in flake.nix so that the diff-test executable can be found.

Key places to review:

flake.nix file.

@sveitser
Copy link
Collaborator

sveitser commented Dec 13, 2024

I think we should avoid this because it leads to cases where one may compile with "release" expecting to run these binaries but due to PATH settings one will still run the debug binaries. Or worse, if some binaries are available on debug and some only on release profile it will just pick up whatever it finds first.

I think we don't need to compile diff test with release or is it too slow with dev or test profile? We now have some optimizations for these profiles too so I think it should be fine.

Copy link
Collaborator

@sveitser sveitser left a comment

Choose a reason for hiding this comment

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

See other comment

@sveitser
Copy link
Collaborator

I think we can change the just file recipes to build the diff test binary to cargo build --profile test --bin diff-test

@philippecamacho
Copy link
Contributor Author

I think we can change the just file recipes to build the diff test binary to cargo build --profile test --bin diff-test

Yes, it works. Done in af6c8c4.

@sveitser sveitser enabled auto-merge (squash) December 13, 2024 13:10
@philippecamacho philippecamacho enabled auto-merge (squash) December 13, 2024 13:16
@philippecamacho philippecamacho merged commit 661298c into main Dec 13, 2024
20 of 21 checks passed
@philippecamacho philippecamacho deleted the fix/update-path-env-variable-for-running-solidity-tests branch December 13, 2024 13:26
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.

3 participants