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

fix json generation #522

Merged
merged 4 commits into from
Feb 19, 2025
Merged

fix json generation #522

merged 4 commits into from
Feb 19, 2025

Conversation

MatusKysel
Copy link
Contributor

@MatusKysel MatusKysel commented Feb 18, 2025

After recent change in spec, we need to run go generate to get tests.json file used for testing in ssv. But because of runtime.Caller(0) generating json outside the repo is impossible. Whit this proposed change we can run it in any directory, where the binary used for generating code is located

@MatusKysel MatusKysel marked this pull request as ready for review February 19, 2025 07:21
Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

I think you should use filepath tools
I think they are not system dependent..

I think currently it is possible to work on spec tests from Windows (never tried it)

@MatusKysel
Copy link
Contributor Author

I think you should use filepath tools I think they are not system dependent..

I think currently it is possible to work on spec tests from Windows (never tried it)

I don't see anything that will not work on windows except permissions. But I am unsure how to fix that

@MatheusFranco99
Copy link
Contributor

@MatusKysel, you've made the change for the qbft and ssv modules, but not for the types one. Is it not tested on your side? I think that, even so, we may adjust it in a similar manner for consistency.

@GalRogozinski
Copy link
Contributor

GalRogozinski commented Feb 19, 2025

@MatusKysel
As far as I know windows uses backslashes (\) while macos/linux use forward slashes (/) like you used.
You honestly tackled me on the permission bit, but chatgpt tells me Windows simply ignores the commands.

Anyhow fileutils is there to take care of all cross compatibility issue, and it is considered a programming best practice to always use such libs for files (at least this is what I was taught as a baby dev)

@MatusKysel
Copy link
Contributor Author

@MatusKysel As far as I know windows uses backslashes () while macos/linux use forward slashes (/) like you used. You honestly tackled me on the permission bit, but chatgpt tells me Windows simply ignores the commands.

Anyhow fileutils is there to take care of all cross compatibility issue, and it is considered a programming best practice to always use such libs for files (at least this is what I was taught as a baby dev)

@GalRogozinski check it one more time, I am actually fixing it
image

Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

I know this isn't on you but can you change permissions to everytime we write file to (0664). This should solve issues when we run generate several times

@GalRogozinski GalRogozinski merged commit 3a9cb8e into main Feb 19, 2025
2 checks passed
@GalRogozinski GalRogozinski deleted the fix-json-generation branch February 19, 2025 14:48
@MatusKysel MatusKysel restored the fix-json-generation branch February 19, 2025 15:49
@MatusKysel MatusKysel deleted the fix-json-generation branch February 19, 2025 17:35
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.

4 participants