-
Notifications
You must be signed in to change notification settings - Fork 55
9p: add files and steps for 9p binary compilation #1051
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: release-4.18
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis pull request integrates the compilation of the Sequence diagram for 9pfs compilation and extractionsequenceDiagram
participant BuildScript as createdisk.sh
participant VM
participant PodmanInVM as Podman (inside VM)
Note over BuildScript, VM: Assumes ARCH=x86_64 and SNC_GENERATE_WINDOWS_BUNDLE!=0
BuildScript->>VM: Calls compile_and_extract_9pfs()
BuildScript->>VM: scp -r 9pfs ./core@VM_IP:/home/core/
BuildScript->>VM: ssh core@VM_IP '...' (execute commands)
activate VM
VM->>PodmanInVM: podman build -t 9pfs-builder .
activate PodmanInVM
PodmanInVM-->>VM: Build Complete
deactivate PodmanInVM
VM->>PodmanInVM: podman create --name extract-temp 9pfs-builder
activate PodmanInVM
PodmanInVM-->>VM: Container Created (extract-temp)
deactivate PodmanInVM
VM->>PodmanInVM: podman cp extract-temp:/src/9pfs/9pfs ./9pfs
activate PodmanInVM
PodmanInVM-->>VM: Binary Copied
deactivate PodmanInVM
VM->>PodmanInVM: podman rm extract-temp
activate PodmanInVM
PodmanInVM-->>VM: Container Removed
deactivate PodmanInVM
VM-->>VM: sudo cp 9pfs /usr/local/bin
VM-->>BuildScript: SSH command completes
deactivate VM
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Hey @redbeam - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider pinning the git clone in the Dockerfile to a specific commit hash or tag for reproducible builds.
- The
compile_and_extract_9pfs
function copies the9pfs
directory to the VM, but the Dockerfile then clones the repository again; perhaps only the Dockerfile needs to be copied.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
405d9fc
to
d9be1e5
Compare
d9be1e5
to
a1461ea
Compare
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.
Hey @redbeam - I've reviewed your changes - here's some feedback:
- Consider building the 9pfs binary outside the target VM to simplify the process within createdisk-library.sh.
- Pinning the 9pfs dependency to a specific commit hash might be fragile; consider using a git tag or submodule.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a1461ea
to
e149f5c
Compare
Have you considered a similar approach to the routes-controller builds? They are done on RH build systems, |
@cfergeau Thanks for the feedback! This was done after consulting with @praveenkumar |
This approach was for POC to make sure it works as expected. Also I think it is for fuse driver instead 9p but if that need kernel headers then better to build using same image which rhel-coreos uses for that release instead based on latest ubi otherwise it will be mismatch for kernel header. |
Yes, this is the FUSE client, since we chose this approach instead of the kernel module one. |
e149f5c
to
67f3324
Compare
67f3324
to
12299e4
Compare
/hold |
9P support for filesharing on Windows: crc-org/crc#4168
Summary by Sourcery
Add support for compiling 9p filesystem binary for Windows file sharing
New Features:
Build:
Chores: