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

soroban rpc make requires cargo for preflight step #415

Closed
wants to merge 2 commits into from

Conversation

sreuland
Copy link
Contributor

started seeing build errors from make build, noticed soroban-rpc requires using its makefile now, there are multiple steps involved in building it that have to be followed.

updated quickstart make and gh workflow to use intermediate dockerfile to run the soroban rpc make.

@tsachiherman
Copy link
Contributor

I think that I understand the problem - but why wouldn't you update the docker in the soroban-tools ? I.e. keep the docker and the code in the same repo, so the hash remain the same.

RUN curl https://sh.rustup.rs -sSf | sh -s -- -y --default-toolchain stable

RUN make build-soroban-rpc
RUN mv soroban-rpc /bin/soroban-rpc
Copy link
Member

@leighmcculloch leighmcculloch Jan 30, 2023

Choose a reason for hiding this comment

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

The goal is to keep the dockerfiles in the respective repos, so ideally we'd update the dockerfile in the soroban-rpc repo so that it can successfully build the entire product instead of duplicating build process here.

Is there a reason we need the file here?

(Note that we do have a horizon file here, but that's only because we couldn't update the horizon file for a past release. Well switch back to the horizon repos file in a future version of quickstart.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I saw the Horizon dockerfile, and seemed like a pattern to follow, @tsachiherman mentioned same - #415 (comment)

so, looks like I'll fix this from the soroban-tools side instead, closing this pr then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did the equivalent change on the soroban-tools dockerfile - stellar/stellar-cli#371

@sreuland
Copy link
Contributor Author

I think that I understand the problem - but why wouldn't you update the docker in the soroban-tools ? I.e. keep the docker and the code in the same repo, so the hash remain the same.

@tsachiherman , I wasn't sure on usage for that Dockerfile, it has some integration setup as it's installing core from debian pkg, is the file deprecated? I could repurpose that, would have to remove the core install, won't work on an arm64 machine.

There's some precedence here for integrating as a client to the external build in a local docker such as Dockerfile.horizon does also, taking similar approach, setting host machine requirements in docker to run the makefile. One other aspect I was curious about is if Cargo.lock target in that makefile could check if rust toolchain exists first and install that if not?

I think we'll need to also apply a similar environment change to have rust toolchain present in pipeline for the soroban-rpc-package-builder.

@sreuland sreuland closed this Jan 31, 2023
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