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

linux-cachyos-hardened: 6.12.5-1 #362

Closed
wants to merge 1 commit into from

Conversation

sm9cc
Copy link
Contributor

@sm9cc sm9cc commented Dec 18, 2024

I noticed that the hardened kernel was outdated but there are patches for 6.12.5.

Signed-off-by: SM9() <me@sm9.dev>
@ptr1337
Copy link
Member

ptr1337 commented Dec 18, 2024

We are currently waiting that linux-hardened gets updated in archlinux - I know the patchset exists, but the config is also an important part
https://archlinux.org/packages/extra/x86_64/linux-hardened/

@1Naim
Copy link
Member

1Naim commented Dec 21, 2024

This seems like a very lousy attempt at rebasing the PKGBUILD.

  1. The PKGBUILD is lazily copied over from the default kernel without default kernel exclusives:
    a. AutoFDO
    b. Conditional clang depends -- .SRCINFO generation is broken with this and it's been decided that only the default kernel will have this because the default kernel defaults to ThinLTO
    c. Defaulting to ThinLTO
    d. Using user-generated config

@1Naim 1Naim closed this Dec 21, 2024
@sm9cc
Copy link
Contributor Author

sm9cc commented Dec 22, 2024

This seems like a very lousy attempt at rebasing the PKGBUILD.

Why?, its not cool to to be a cunt, it is better to be clear and constructive, that way I can improve any commits later on.

1. The PKGBUILD is lazily copied over from the default kernel without default kernel exclusives:
  • Please tell me the none "lazy" way then, why would I make a commit at all if I was trying to be "lazy"?
  • All of the kernels PKGBUILD's are based on main PKGBUILD, a simple concept that I followed here.
   a. AutoFDO

?

   b. Conditional clang depends -- .SRCINFO generation is broken with this and it's been decided that only the default kernel will have this because the default kernel defaults to ThinLTO

Not sure what you mean, why does .SRCINFO matter, are you submitting this to the aur?

   c. Defaulting to ThinLTO

Whats wrong with ThinLTO, can you be a little clearer?

   d. Using user-generated config

Obviously this part is wrong as ptr1337 has already mentioned, once the correct config was available I was going to amend my commit.


  • The kernel built fine on my end with this PKGBUILD.
  • I thought clang / AutoFDO was the new default for all kernels.

I look forward to hearing your (none cuntish) reply that hopefully clears things up a little more.

Thanks

Copy link
Member

@1Naim 1Naim left a comment

Choose a reason for hiding this comment

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

Here are the reviews if I haven't made myself clear.

@@ -106,18 +105,17 @@ _use_auto_optimization=${_use_auto_optimization-y}
# "full: uses 1 thread for Linking, slow and uses more memory, theoretically with the highest performance gains."
# "thin: uses multiple threads, faster and uses less memory, may have a lower runtime performance than Full."
# "none: disable LTO
_use_llvm_lto=${_use_llvm_lto-none}
_use_llvm_lto=${_use_llvm_lto-thin}
Copy link
Member

Choose a reason for hiding this comment

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

Only the default kernel defaults to ThinLTO, the rest stick to GCC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this, can you explain? thanks.

# This was added to facilitate https://github.com/CachyOS/linux-cachyos/issues/286
_use_gcc_suffix=${_use_gcc_suffix-}
# Enabled by default to show the difference between LTO kernels and GCC kernels
_use_gcc_suffix=${_use_gcc_suffix-y}
Copy link
Member

Choose a reason for hiding this comment

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

This is only meant to be used by the default kernel to not break the naming scheme for the rest of the kernels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, I guess the whole concept here can be improved on.

_pkgsuffix="cachyos-${_cpusched}-lto"
elif [ "$_use_llvm_lto" = "none" ] && [ -z "$_use_kcfi" ] && [ "$_use_gcc_suffix" = "y" ]; then
_pkgsuffix="cachyos-${_cpusched}-gcc"
# Enable AUTOFDO_CLANG for the first compilation to create a kernel, which can be used for profiling
Copy link
Member

Choose a reason for hiding this comment

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

AutoFDO isn't used by any other kernel except for the default and rc so there's not reason to add this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whether its used or not is the decision of whoever is building the kernel, also, keeping it there keeps things consistent.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to add it in the name of consistency, then you should add it for the other PKGBUILDs too. The default kernel doesn't need to follow the other PKGBUILDs since it's already different in many ways (ThinLTO default, AutoFDO, more soon)

Comment on lines +164 to +167
if _is_lto_kernel && [ "$_use_lto_suffix" = "y" ]; then
_pkgsuffix=cachyos-hardened-lto
elif ! _is_lto_kernel && [ "$_use_gcc_suffix" = "y" ]; then
_pkgsuffix=cachyos-hardened-gcc
Copy link
Member

Choose a reason for hiding this comment

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

Why are you even hardcoding hardened here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it was not tagged with "-hardened" when I built it, so I added this, also, this is the PKGBUILD for the hardened kernel, why does it matter?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't really matter much because -hardened uses its own configs, but ideally there should only be one PKGBUILD. We have much more in this repo to facilitate the building process to our repositories. Just to add another point, it keeps consistency :) with the rest of the PKGBUILDs that does this.

Comment on lines +639 to +641
if _is_lto_kernel; then
depends+=(clang llvm lld)
fi
Copy link
Member

Choose a reason for hiding this comment

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

.SRCINFO doesn't respect any conditionals and will add this regardless. Since the other kernels don't default to LTO, we've decided to not add this for the other PKGBUILDs. Please see #346.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, although again, I thought all kernels default to LTO / Clang now, if not, would you mind explaining why not? Thanks

Copy link
Member

Choose a reason for hiding this comment

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

#286. IMO it's to reduce the resources used when building kernels as the other kernel variants aren't used as much if at all compared to the default kernel. Other than that, another mass migration would need to take place by adding provides() replaces() to the PKGBUILDs temporarily which we're not quite fond of doing.

@1Naim
Copy link
Member

1Naim commented Dec 22, 2024

Not sure what you mean, why does .SRCINFO matter, are you submitting this to the aur?

Yes, and devtools also utilizies .SRCINFO to grab package metadata.

Whats wrong with ThinLTO, can you be a little clearer?

Nothing, it's just that the other kernels will default to GCC, and the changes you've made in this PR obviously goes against that decision.

once the correct config was available I was going to amend my commit

There's a feature to draft pull requests in GitHub. How am I (or any other person other than you) supposed to know if this PR is ready or if there will be any other future changes?

@sm9cc
Copy link
Contributor Author

sm9cc commented Dec 22, 2024

Not sure what you mean, why does .SRCINFO matter, are you submitting this to the aur?

Yes, and devtools also utilizies .SRCINFO to grab package metadata.

Understood.

Whats wrong with ThinLTO, can you be a little clearer?

Nothing, it's just that the other kernels will default to GCC, and the changes you've made in this PR obviously goes against that decision.

I thought clang was the new default for all kernels, excuse me if I am wrong here.

once the correct config was available I was going to amend my commit

There's a feature to draft pull requests in GitHub. How am I (or any other person other than you) supposed to know if this PR is ready or if there will be any other future changes?

You are correct, I should have submitted a draft.

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