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: cross build ci required for releases + remove const_format #1643

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

KolbyML
Copy link
Member

@KolbyML KolbyML commented Jan 17, 2025

What was wrong?

There was a bug in #1615 which caused https://github.com/ethereum/trin/actions/runs/12834319209/job/35791388762 release ci to fail, on trying to build.

How was it fixed?

I removed const_format cause it wasn't needed and also fixed some indexing

I tested on my forked github repo and it works

release: https://github.com/KolbyML/trin/releases
release branch: https://github.com/KolbyML/trin/tree/fix-cross-build
ci working: https://github.com/KolbyML/trin/actions/runs/12835484962/job/35796339052

image

I tested the binary built locally

image

and the short commit matches

@KolbyML KolbyML self-assigned this Jan 17, 2025
Copy link
Collaborator

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

Nice, the only thing I can think of is to make the environment variable names their own variables, so we can just use the variable across files and not worry about possible spelling errors breaking something. Though, it seems like these are the only places these env vars will be used, so it might end up being too much overhead for not much utility. Anyways, leaving that call up to you

@KolbyML
Copy link
Member Author

KolbyML commented Jan 17, 2025

Nice, the only thing I can think of is to make the environment variable names their own variables, so we can just use the variable across files and not worry about possible spelling errors breaking something. Though, it seems like these are the only places these env vars will be used, so it might end up being too much overhead for not much utility. Anyways, leaving that call up to you

I am open to it being done in a future PR, I want to deploy the release now and CI takes 10 minutes xD

@KolbyML KolbyML merged commit 0b54f24 into ethereum:master Jan 17, 2025
11 checks passed
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