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

kernel plugin: properly generate Ubuntu kernel .config #4210

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

arighi
Copy link
Contributor

@arighi arighi commented Jun 13, 2023

Recent Ubuntu kernels (>= Jammy) are now using a new format to store their .config, called annotations [1].

The kernel plugin is trying to assemble a .config using the old config chunks stored in debian.<kernel>/config/* that are not available anymore in the kernels that are using this new annotations model.

Instead of trying to manually assemble the config we should rely on debian/rules genconfigs that is more portable across different Ubuntu kernels and releases.

[1] https://lists.ubuntu.com/archives/kernel-team/2023-June/140230.html

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint?
  • Have you successfully run pytest tests/unit?

@kubiko
Copy link
Contributor

kubiko commented Jul 3, 2023

@arighi thank you! this looks good.
Can you please ensure tests pass as well?
Would this work for any kernel done from Focal onwards?

@sergiusens
Copy link
Collaborator

@arighi can you take a look at @kubiko's question please?

@arighi
Copy link
Contributor Author

arighi commented Jul 12, 2023

@arighi can you take a look at @kubiko's question please?

Working on it. I've just updated my pull request, the last one should fix all the kernel test cases.

@kubiko about the support for older kernels, yes, it will work with any kernel back to focal (and even before focal), the target debian/rules genconfigs has been supported since ... forever :) and it's the most reliable way to generate kernel .config for all the supported architectures and flavours, also with older kernels/releases.

@xnox
Copy link
Contributor

xnox commented Jul 17, 2023

@gkoh @kubiko how can arighi fix the pull request CI, if the CI doesn't start and nobody is approving it? Can workflows please be started? or should arighi submit some minor whitespace change to be allowed to trigger workflows on merge proposals?

@gkoh
Copy link

gkoh commented Jul 17, 2023

@gkoh @kubiko how can arighi fix the pull request CI, if the CI doesn't start and nobody is approving it? Can workflows please be started? or should arighi submit some minor whitespace change to be allowed to trigger workflows on merge proposals?

@xnox
This needs someone in the snapcraft team to approve.
I believe the default Github project configuration is that first-time contributors need manual approval for workflows to execute.

@gkoh
Copy link

gkoh commented Jul 19, 2023

@gkoh @kubiko how can arighi fix the pull request CI, if the CI doesn't start and nobody is approving it? Can workflows please be started? or should arighi submit some minor whitespace change to be allowed to trigger workflows on merge proposals?

@xnox This needs someone in the snapcraft team to approve. I believe the default Github project configuration is that first-time contributors need manual approval for workflows to execute.

@sergiusens Can you help move this PR along?

@xnox
Copy link
Contributor

xnox commented Jul 31, 2023

@arighi workflows were re-run, but there are now failing tests again, and rebase needed.

@arighi
Copy link
Contributor Author

arighi commented Jul 31, 2023

@arighi workflows were re-run, but there are now failing tests again, and rebase needed.

I'm trying to setup a dev environment to verify all these tests locally. I'm wondering if there's some documentation on how to verify everything without relying on the CI here in github, I've followed all the steps here (https://github.com/snapcore/snapcraft/blob/main/HACKING.md) and I have created a local dev environment correctly, but then the tests are failing even without my patch applied... I'll investigate more, I need to start from a local environment where everything works, then I'll apply my patch, re-run all the tests and if everything is good I'll rebase+push again.

@kubiko
Copy link
Contributor

kubiko commented Jul 31, 2023

@xnox this is now rebased, I will run a test build to make sure things are still working

@kubiko
Copy link
Contributor

kubiko commented Jul 31, 2023

@arighi I have struggled with this as well, not well documented. Here are my notes
on focal

$ sudo apt install -y libapt-pkg-dev python3-pip python3-virtualenv 
$ cd <snapcraft project root>
$ virtualenv .venv
$ source .venv/bin/activate
$ pip install -U pip
$ pip install -r requirements-devel.txt
$ pytest tests/unit/parts/plugins/test_kernel.py ; pytest tests/legacy/unit/plugins/v2/test_kernel.py

As part of the snapcraft release there are more tests, those can be run with ./runtests.sh
I have setup LP build for this branch here:
https://launchpad.net/~ondrak/+snap/snapcraft-kernel-initrd-split

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2023

Codecov Report

Merging #4210 (f8b46de) into main (5ec6572) will decrease coverage by 0.14%.
The diff coverage is n/a.

❗ Current head f8b46de differs from pull request most recent head 989b49d. Consider uploading reports for the commit 989b49d to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #4210      +/-   ##
==========================================
- Coverage   89.11%   88.97%   -0.14%     
==========================================
  Files         300      295       -5     
  Lines       20623    20171     -452     
==========================================
- Hits        18378    17948     -430     
+ Misses       2245     2223      -22     
Files Changed Coverage Δ
snapcraft_legacy/plugins/v2/_kernel_build.py 86.66% <ø> (ø)

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@arighi
Copy link
Contributor Author

arighi commented Aug 3, 2023

@kubiko it looks like the tests are finally passing now. Is there anything special to do or do I just need to wait for a review/approval? Thanks.

@kubiko
Copy link
Contributor

kubiko commented Aug 4, 2023

@arighi that should be all. you just need snapcraft team review now

@xnox
Copy link
Contributor

xnox commented Aug 8, 2023

I am not sure how kernel team can maintain this, without powers to run CI or merge changes.

@kubiko
Copy link
Contributor

kubiko commented Aug 8, 2023

@xnox if we are open to improving Ubuntu upstream kernel packaging, I'd propose allowing running
debian/rules clean genconfigs out of the source tree, or providing an output directory. So we do not need to copy over files and clean the source tree after running the steps.

@sergiusens
Copy link
Collaborator

@xnox moving this repo to the canonical org is probably long overdue, that should allow running tests without this churn. In any case, until that happens, please give me a list of folks so I can add as contributors.

sergiusens
sergiusens previously approved these changes Aug 11, 2023
Copy link
Contributor

@kubiko kubiko left a comment

Choose a reason for hiding this comment

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

LGTM

@xnox
Copy link
Contributor

xnox commented Aug 15, 2023

@arighi

commit 5685218369e6bc93c1fd72d8d59616777d0b0e1b
Merge: f8b46de068 8758571f08
Author: Andrea Righi <r....a@gmail.com>
Date:   Mon Aug 14 11:55:48 2023 +0200

    Merge branch 'main' into kernel-config-fix

It seems the merge was done using @gmail.com address, instead of your @canonical.com address which is now tripping up the cla check. I don't know if you can rewrite that merge commit with @canonical.com address. Or if this was done using github UI to like change github's default email to @canonical.com if it isn't that for things like these.

@arighi
Copy link
Contributor Author

arighi commented Aug 16, 2023

@arighi

commit 5685218369e6bc93c1fd72d8d59616777d0b0e1b
Merge: f8b46de068 8758571f08
Author: Andrea Righi <r....a@gmail.com>
Date:   Mon Aug 14 11:55:48 2023 +0200

    Merge branch 'main' into kernel-config-fix

It seems the merge was done using @gmail.com address, instead of your @canonical.com address which is now tripping up the cla check. I don't know if you can rewrite that merge commit with @canonical.com address. Or if this was done using github UI to like change github's default email to @canonical.com if it isn't that for things like these.

Hm... not sure why it used my gmail address, my default email in github is already the one @canonical.com and for the commit I've also used @canonical.com: Author: Andrea Righi <andrea.righi@canonical.com>. I guess when I click "Create merge request" here on github is still using the gmail one, because it's the one associated to my account...? I'll investigate a bit.

Recent Ubuntu kernels (>= Jammy) are now using a new format to store
their .config, called annotations [1].

The kernel plugin is trying to assemble a .config using the old config
chunks stored in `debian.<kernel>/config/*` that are not available
anymore in the kernels that are using this new annotations model.

Instead of trying to manually assemble the config we should rely on
`debian/rules genconfigs` that is more portable across different Ubuntu
kernels and releases.

[ Thanks to Guo-Rong for fixing all the test case failures. ]

[1] https://lists.ubuntu.com/archives/kernel-team/2023-June/140230.html

Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
@xnox
Copy link
Contributor

xnox commented Aug 17, 2023

@sergiusens kernel team approves of this PR. please approve & merge this on our behalf due to lack of ACLs on this.

@sergiusens sergiusens merged commit da90067 into canonical:main Aug 23, 2023
12 checks passed
jardon pushed a commit to jardon/snapcraft that referenced this pull request Sep 6, 2023
Recent Ubuntu kernels (>= Jammy) are now using a new format to store
their .config, called annotations [1].

The kernel plugin is trying to assemble a .config using the old config
chunks stored in `debian.<kernel>/config/*` that are not available
anymore in the kernels that are using this new annotations model.

Instead of trying to manually assemble the config we should rely on
`debian/rules genconfigs` that is more portable across different Ubuntu
kernels and releases.

[1] https://lists.ubuntu.com/archives/kernel-team/2023-June/140230.html

Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
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.

6 participants