fix: Allow detect_missing_libraries only while building
#803
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The purpouse of this PR is to remove the possiblitityto use the detect_missing_libraries from the config toml file, because it does not makes sense to havie it there. The way this flags works, is to show a list of libraries that are missing, and it does not output any bytecode, but there some other scenarios where this could happen, so the best way to fix it is to remove this from all the commands (script, test, create) which is the same as disable it by default in all places and enabling it only for the build. finally, if there is someone still using this flag in the config, we show a warning mentioning that this is not the way to go. fix
MBerguer
force-pushed
the
tincho/detect-missing-libraries
branch
from
02d9b9c to
4667606
Compare
elfedy
reviewed
Jan 6, 2025
- Removing the warning for all the commands besides build - Cleaning up the three tests
zk_detect_missing_libraries since we have this naming convention
MBerguer
changed the title
detect_missing_libraries only while buildingdetect_missing_libraries only while building
Rename it back so the compiler understand fixing the test to assert for the missing library
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What 💻
The purpose of this PR is to remove the possibility of using the
detect_missing_librariesfrom the config toml file because it does not make sense to have it there. This flag works by showing a list of missing libraries. It does not output any bytecode, but there are some other scenarios where this could happen, so the best way to fix it is to remove this from all the commands (script, test, create), which is the same as disabling it by default in all places and enabling it only for the build.Why ✋
We were experiencing issues with this flag (#710) because it was enabled for all the available subcommands.
And the error we were getting down the line was nearly understandable. Now, the user will still get the problem eventually, but if he looks above in the stack trace, there will be a message explaining why this happened, so he can change the config file and continue.
Evidence 📷
Notes 📝