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

release: install multiple kernels if specified #1566

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

asomers
Copy link
Member

@asomers asomers commented Jan 5, 2025

If release.conf sets KERNEL to a space-separate list, release.sh will now install all of them into the release media, building separate .txz archives for each.

Sponsored by: ConnectWise

To eliminate the double negative.

MFC after:	Never
Sponsored by:	ConnectWise
If release.conf sets KERNEL to a space-separate list, release.sh will
now install all of them into the release media, building separate .txz
archives for each.

Sponsored by:	ConnectWise
@asomers asomers requested review from brd and evadot January 5, 2025 17:14
@emaste
Copy link
Member

emaste commented Jan 7, 2025

Eliminating the double negative LGTM, will look at the other change when I have a moment

@asomers
Copy link
Member Author

asomers commented Jan 17, 2025

@markjdb this is what I was talking about in the video chat today. This PR gets both kernels onto the install media. One wart is that they also both get installed into /boot, bloating the CD image. IMHO that shouldn't be a barrier to merge, because the 2nd kernel is disabled by default.
Any freebsd-update changes will come separately.

@brooksdavis
Copy link
Contributor

Hmm, on CheriBSD we've added individual kernels to release/scripts/make-manifest.sh so they are in the distribution menu in the installer (this is decidedly not generalized). I think that's somewhat related, but not quite the same thing. I don't think we had to make any changes to Makefile.inc1 for this, but these change might well be simplifying?

CC @jrtc27

@asomers
Copy link
Member Author

asomers commented Jan 17, 2025

@brooksdavis the changes that I made to Makefile.inc1 are entirely about renaming the variable to eliminate a double negative. That change is segregated to the first commit. The second is the actual functional change. And yes it does result in the extra kernels being in the install menu.

@jrtc27
Copy link
Contributor

jrtc27 commented Jan 17, 2025

Are you sure this works? distributekernel / packagekernel is what creates the tarballs, but you've gone and messed with installworld/installkernel/distribution, which is solely for building the live environment the installer runs in. Which is the exact opposite of what needs to change.

@asomers
Copy link
Member Author

asomers commented Jan 17, 2025

Are you sure this works? distributekernel / packagekernel is what creates the tarballs, but you've gone and messed with installworld/installkernel/distribution, which is solely for building the live environment the installer runs in. Which is the exact opposite of what needs to change.

I can verify that this change works. It results in the alternate kernel being added to the installation menu in the CD. And it also results in the alternate kernel being added to the CD's own /boot, which is not ideal.

@jrtc27
Copy link
Contributor

jrtc27 commented Jan 17, 2025

Hmm, on CheriBSD we've added individual kernels to release/scripts/make-manifest.sh so they are in the distribution menu in the installer (this is decidedly not generalized).

There are defaults in there. Without modifying the script it'll default to a blank description and selected for install by default. Including the debug symbols tarballs, which is unlikely to be desired, even if you want the kernels.

@jrtc27
Copy link
Contributor

jrtc27 commented Jan 17, 2025

Are you sure this works? distributekernel / packagekernel is what creates the tarballs, but you've gone and messed with installworld/installkernel/distribution, which is solely for building the live environment the installer runs in. Which is the exact opposite of what needs to change.

I can verify that this change works. It results in the alternate kernel being added to the installation menu in the CD. And it also results in the alternate kernel being added to the CD's own /boot, which is not ideal.

Something's gone wrong then, because it should not do so.

@asomers
Copy link
Member Author

asomers commented Jan 17, 2025

Hmm, on CheriBSD we've added individual kernels to release/scripts/make-manifest.sh

@brooksdavis could you please share that change? It seems to me that the natural way to add more kernels is in the _invocation_of make-manifest.sh, which is in release/Makefile, not within make-manifest.sh itself.

@jrtc27
Copy link
Contributor

jrtc27 commented Jan 17, 2025

Are you sure this works? distributekernel / packagekernel is what creates the tarballs, but you've gone and messed with installworld/installkernel/distribution, which is solely for building the live environment the installer runs in. Which is the exact opposite of what needs to change.

I can verify that this change works. It results in the alternate kernel being added to the installation menu in the CD. And it also results in the alternate kernel being added to the CD's own /boot, which is not ideal.

Something's gone wrong then, because it should not do so.

I wonder if the use of ${MAKE} in release/Makefile ends up passing down all the random flags passed by the parent process to the child process, and so it's your modification of RELEASE_RMAKEFLAGS that effectively messes with everything. I'd guess that, were you to just change INSTALLEXTRAKERNELS's default in release/Makefile, without changing release/Makefile, it would be broken. Probably also the changes you've made to release/Makefile itself don't matter when called from release.sh...

Anyway, regardless, this is quite broken.

@jrtc27
Copy link
Contributor

jrtc27 commented Jan 17, 2025

Hmm, on CheriBSD we've added individual kernels to release/scripts/make-manifest.sh

@brooksdavis could you please share that change? It seems to me that the natural way to add more kernels is in the _invocation_of make-manifest.sh, which is in release/Makefile, not within make-manifest.sh itself.

https://github.com/CTSRD-CHERI/cheribsd/commits/main/release/scripts/make-manifest.sh

@asomers
Copy link
Member Author

asomers commented Jan 17, 2025

Hmm, on CheriBSD we've added individual kernels to release/scripts/make-manifest.sh

@brooksdavis could you please share that change? It seems to me that the natural way to add more kernels is in the _invocation_of make-manifest.sh, which is in release/Makefile, not within make-manifest.sh itself.

https://github.com/CTSRD-CHERI/cheribsd/commits/main/release/scripts/make-manifest.sh

Oh, that just provides descriptions for the alternate kernels in the installer. It's the invocation of make-manifest.sh, in release/Makefile, that puts the alternate kernels there in the first place. By default, without customized descriptions.

@asomers
Copy link
Member Author

asomers commented Jan 17, 2025

I wonder if the use of ${MAKE} in release/Makefile ends up passing down all the random flags passed by the parent process to the child process, and so it's your modification of RELEASE_RMAKEFLAGS that effectively messes with everything.

Yes, I think that's exactly what's happening. The change to RELEASE_RMAKEFLAGS is the key part. Would you have done it differently?

@jrtc27
Copy link
Contributor

jrtc27 commented Jan 17, 2025

I wonder if the use of ${MAKE} in release/Makefile ends up passing down all the random flags passed by the parent process to the child process, and so it's your modification of RELEASE_RMAKEFLAGS that effectively messes with everything.

Yes, I think that's exactly what's happening. The change to RELEASE_RMAKEFLAGS is the key part. Would you have done it differently?

Use a different name in release/Makefile to capture that it's only for the tarballs? Then change the distributekernel + packagekernel invocations to pass that as INSTALLEXTRAKERNELS, and leave the other recursive makes alone.

As it stands the changes to release/Makefile are completely useless, since INSTALLEXTRAKERNELS already gets passed down if provided. So there's no point making them unless it's to enable different behaviour to what you can already get with that (which as you note is what you want, because you don't want all the kernels to be part of the live image).

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.

4 participants