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

neonvm-kernel: kernel build improvements #1155

Closed
wants to merge 1 commit into from

Conversation

mikhail-sakhnov
Copy link
Contributor

@mikhail-sakhnov mikhail-sakhnov commented Nov 22, 2024

Fix readme file for kernel builder
Update ubuntu to LTS
Rename kernel config file

Copy link
Member

@sharnoff sharnoff left a comment

Choose a reason for hiding this comment

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

nice, thanks!

neonvm-kernel/README.md Outdated Show resolved Hide resolved
@mikhail-sakhnov mikhail-sakhnov force-pushed the misha/fix/kernel-readme branch 2 times, most recently from 6d257c5 to 5528d42 Compare November 26, 2024 09:26
@mikhail-sakhnov mikhail-sakhnov marked this pull request as ready for review November 26, 2024 09:50
@mikhail-sakhnov mikhail-sakhnov changed the title neonvm-kernel: fix readme to mention architecture neonvm-kernel: kernel build improvements Nov 26, 2024
@Omrigan
Copy link
Contributor

Omrigan commented Nov 26, 2024

Closes #1154?

neonvm-kernel/Dockerfile.kernel-builder Outdated Show resolved Hide resolved
@@ -26,18 +26,19 @@ with the following sequence of actions:
```
2. Then, inside the container, run:
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Should it be explained in this section that you need to do upgrade twice: once for x86, and once for arm64?
  2. Would it be possible to do this:
export KERNEL_ARCH=...
export OLD_VERSION=...
export NEW_VERSION-...

in the first step, and then pass the variables to docker run with --env, and overall make it so the remaining commands don't need to be modified?

@Omrigan Omrigan assigned Omrigan and mikhail-sakhnov and unassigned Omrigan Nov 26, 2024
@mikhail-sakhnov
Copy link
Contributor Author

mikhail-sakhnov commented Nov 26, 2024

Closes #1154?

@Omrigan No, it doesn't, it was originally a quick fix in the readme, later we decided with @sharnoff to rename files as well to have arch in sync, while renaming files I found that ubuntu version we use has repo ran out of the support, now I added your suggested improvals and hope that would be the last iteration of "fixing readme" 😃

UPD: it actually does, I originally looked into separate kernel issue, my fault.

Comment on lines +34 to +35
cp /host/linux-config-$ARCH-$OLD_VERSION .config # Copy current config in
make ARCH=$KERNEL_ARCH menuconfig
Copy link
Member

Choose a reason for hiding this comment

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

If these environment variables are defined on the host, are they actually available inside the container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I passed them as a build-arg 🤦 Moved to the docker run -e

@mikhail-sakhnov
Copy link
Contributor Author

Closing the PR because I accidentally rebased and pushed version from the other dev machine, and now some changes were losh 🤷 . Will re-open from the correct branch during normal working hours.

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