Skip to content
This repository has been archived by the owner on Sep 9, 2024. It is now read-only.

fix docker container #136

Merged
merged 11 commits into from
Feb 6, 2023
Merged

fix docker container #136

merged 11 commits into from
Feb 6, 2023

Conversation

chunningham
Copy link
Member

Description

Docker builds have been broken by the rate limiting of protoc builds from the Github api stemming from it's (unavoidable) use in libp2p, as well as some other issues. This PR swaps to a branch of libp2p which removes the dependancy on protoc in favour of a pure rust impl. This is not a perfect fix, once the corresponding libp2p PR is merged this branch will disappear and another PR will be needed to go back to libp2p trunk. Another option is to fully remove libp2p for now, but this would entail a number of breaking changes.

Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Diligence Checklist

(Please delete options that are not relevant)

  • This change requires a documentation update
  • I have included unit tests
  • I have updated and/or included new integration tests
  • I have updated and/or included new end-to-end tests
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@chunningham chunningham marked this pull request as ready for review February 2, 2023 11:48
Dockerfile Outdated Show resolved Hide resolved
Copy link

@krhoda krhoda left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I was surprised to see the change from the pattern:
format!("{}", n) to format!("{n}"). Is there any reason for the change, should I follow that convention in the future?

I pulled down the branch planning to run my own instance of the Docker container, but when I tried to build it by running this from the root of the repo:

$ docker build -t kepler .

But I hit an error:

#10 334.6 qemu: uncaught target signal 11 (Segmentation fault) - core dumped
#10 334.6 error: could not compile `termcolor`
#10 334.6
#10 334.6 Caused by:
#10 334.6   process didn't exit successfully: `rustc --crate-name termcolor --edition=2018 /home/rust/.cargo/registry/src/github.com-1ecc6299db9ec823/termcolor-1.2.0/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -C metadata=a7a709df87c9689f -C extra-filename=-a7a709df87c9689f --out-dir /tmp/cargo-installxkX3TF/x86_64-unknown-linux-musl/release/deps --target x86_64-unknown-linux-musl -L dependency=/tmp/cargo-installxkX3TF/x86_64-unknown-linux-musl/release/deps -L dependency=/tmp/cargo-installxkX3TF/release/deps --cap-lints allow` (signal: 11, SIGSEGV: invalid memory reference)

Is this a known issue (like with m1 Macs for instance)? Or is this something we need to work around before approval?

Copy link

@krhoda krhoda left a comment

Choose a reason for hiding this comment

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

The Build issue is M1 specific so I can't go any further, I'm approving but if anyone else can test the container locally, that would be useful.

@chunningham
Copy link
Member Author

I think clippy has been updated to have new rules, all the format! changes were clippy warnings which caused failed checks

Copy link
Member

@sbihel sbihel left a comment

Choose a reason for hiding this comment

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

Kelsey is right, I don't thing you have test the docker build?

@sbihel
Copy link
Member

sbihel commented Feb 3, 2023

The Build issue is M1 specific so I can't go any further

BTW you can build most images for x86 with docker build . --platform linux/amd64 (and the new Rosetta integration works well). There are still some hiccups, as with this project, with some code binding, but it can give you an idea if it should work.

@chunningham chunningham merged commit 6330ef8 into main Feb 6, 2023
@chunningham chunningham deleted the fix/dockerfile branch February 6, 2023 15:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants