-
Notifications
You must be signed in to change notification settings - Fork 1
fix: Docker build on macOS #32
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
base: main
Are you sure you want to change the base?
Conversation
… tools in image Mounting empty host directories to these paths hides the installed tools, causing 'cargo/forge not found' errors.
pdptool is compiled for the container environment and cannot execute on the host due to platform/library incompatibilities. Run it via docker exec and use mounted fast-storage for test files.
pdptool runs inside the container and must connect to localhost:4702 (container internal port), not the dynamically allocated host port.
|
|
pdptool was also placed in the container because cross-platform issues aren't limited to macOS. The same problem might occur when using it on Ubuntu versions below 24.04. |
|
I have tried |
The other steps worked too |
| WORKDIR /workspace | ||
|
|
||
| # Define volumes for external access | ||
| VOLUME ["/home/foc-user/.cargo", "/home/foc-user/.rustup", "/home/foc-user/go", "/var/tmp/filecoin-proof-parameters"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed? This is good for caching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works when there is data present. The first time you run the mount, it will clear these directories, causing the toolchain to be lost. I added the critical mappings in a subsequent commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true, but it cleans up only the first time. The caches are for when the curio or lotus codebase is actively being changed and rebuilt frequently. In those instances, re-init is not needed, and these caches would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cargo bin directory was overwritten, so this binary will never exist unless it is manually downloaded on the host machine.
There are two commits here: one temporarily disables everything, and the second maps according to what's actually usable. The -v parameter plays a crucial role. Therefore, I've removed it here because I think it's meaningless.
However, I must admit that some Rust binaries are not cached. The rest, such as go pkg and rust registry, are cached.
| "run".to_string(), | ||
| "-u".to_string(), | ||
| "foc-user".to_string(), | ||
| "--rm".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if the intent really is:
- "do not use
foc-userfor building" - "remove" the container, lose logs
I don't disagree with them, but would like to understand safety argument for removing -u foc-user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --rm option is used because if cargo run --build lotus fails, subsequent runs will save the changes, requiring manual cleanup. Error logs will be output upon exit, so there's no need to worry.
The -u foc-user was removed because you added -u gid:uid repeatedly later, so this redundant part was removed.
| .parse()?; | ||
| // When running pdptool inside the container, use the container's internal port (4702) | ||
| // not the host-mapped port | ||
| let service_url = "http://localhost:4702"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not hardcode stuff. What's wrong with host-mapped port here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because pdptool was moved inside the container, so a fixed internal port was used.
The reason for moving pdptool internally is that the original logic allowed programs compiled inside the container to be used externally, which was not as expected and could cause problems in most environments.
| let run_id = context.run_id(); | ||
| let container_name = format!("foc-{}-curio-{}", run_id, sp_index); | ||
|
|
||
| let args = [ | ||
| "exec", | ||
| &container_name, | ||
| "/usr/local/bin/lotus-bins/pdptool", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure testing from within curio container constitutes "healthy" properly, especially given public. Would rather have foc-builder run pdptool for stricter testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I will put pdptool in foc-builder and keep testing service_url externally.
| /// Returns the appropriate YugabyteDB tarball URL for the current platform: | ||
| /// - el8-aarch64 for ARM64 systems (Apple Silicon, AWS Graviton, etc.) | ||
| /// - linux-x86_64 for x86_64 systems | ||
| fn get_default_yugabyte_url() -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addition is a "feature update". The S3 fallback has to be changed accordingly in the code as well. Please check that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean by S3 fallback here? But I do see a place I forgot to update.
foc-devnet/scripts/cache-artifacts.sh
Line 27 in 629f8e8
| # Cache Yugabyte tarball |
redpanda-f
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main changes I’d request before merge:
- Fix Yugabyte arch detection (runtime vs compile-time). Also, fix the S3 fallback for downloads
- Re-evaluate removal of
-u foc-user. - Avoid hardcoded ports and paths. Hoist them into constants if really needed.
- Change from compile-time cfg!() to runtime std::env::consts::ARCH - Update cache-artifacts.sh to detect ARM64/x86_64 at runtime
#30