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

autoPatchelfHook: add keep_libc flag #332617

Merged
merged 3 commits into from
Aug 14, 2024
Merged

Conversation

squalus
Copy link
Member

@squalus squalus commented Aug 6, 2024

Description of changes

  • Add keep_libc flag to disable the default libc handling. Intended to be used by systemd.
  • Add autoPatchelfFlags to autoPatchelfHook for passing arguments to the autoPatchelf script

This reverts part of the change made in #307068 / 80be926.

Fixes #332533

Relevant: #325126

cc @ElvishJerricco @layus

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@squalus squalus requested a review from layus as a code owner August 6, 2024 01:26
@ElvishJerricco
Copy link
Contributor

ElvishJerricco commented Aug 6, 2024

This will break systemd initrd, which relies on leaving glibc in the rpath of binaries that had it before the autoPatchelfHook (actually, I think it also breaks scripted initrd).

- Add keep_libc flag to disable the default libc handling. Intended
  to be used by systemd.
- Add autoPatchelfFlags to autoPatchelfHook for passing arguments to
  the autoPatchelf script

This reverts part of the change made in NixOS#307068 / 80be926.

Fixes NixOS#332533
@squalus squalus changed the title autoPatchelfHook: revert libc dependency handling autoPatchelfHook: add keep_libc flag Aug 6, 2024
@squalus
Copy link
Member Author

squalus commented Aug 6, 2024

Updated to accommodate systemd's requirement for different libc handling.

@ofborg ofborg bot requested review from flokli and kloenk August 6, 2024 19:55
@squalus
Copy link
Member Author

squalus commented Aug 10, 2024

Fixes #333710

Copy link
Contributor

@ElvishJerricco ElvishJerricco left a comment

Choose a reason for hiding this comment

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

Other than updating the comment, this looks good to me. I think we can take it out of draft and once the comment is updated we can merge.

Comment on lines +275 to +277
elif is_libc and not keep_libc:
was_found = True
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add to the comment above to include this condition?

@ElvishJerricco ElvishJerricco marked this pull request as ready for review August 12, 2024 22:53
@ElvishJerricco ElvishJerricco requested a review from a team as a code owner August 12, 2024 22:53
@squalus
Copy link
Member Author

squalus commented Aug 14, 2024

Added some new comments and adjusted the flag description. I tried to make it easier to understand. Feel free to suggest new language if anything comes to mind.

@ElvishJerricco
Copy link
Contributor

@squalus Well, originally, that comment was written to have one numbered bullet point for each of the conditions; the idea being to explain step by step what the loop is doing. So I would have preferred adding another bullet point and clarifying why each of the two libc steps is where it is in the process. Maybe something more like this?

            # 1. If a candidate is an absolute path, it is already a
            #    valid dependency if that path exists, and nothing needs
            #    to be done. It should be an error if that path does not exist.
            # 2. If a candidate is found within libc, it should be dropped
            #    and resolved automatically by the dynamic linker, unless
            #    keep_libc is enabled.
            # 3. If a candidate is found in our library dependencies, that
            #    dependency should be added to rpath.
            # 4. If all of the above fail, libc dependencies should still be
            #    considered found. This is in contrast to step 2, because
            #    enabling keep_libc should allow libc to be found in step 3
            #    if possible to preserve its presence in rpath.

I dunno. Your call. Whatever you're most happy with is good with me. Just wanted to offer an explanation for why the bullet points were listed that way.

@squalus
Copy link
Member Author

squalus commented Aug 14, 2024

@ElvishJerricco Updated to use your suggested language. I think that's more clear. Much appreciated.

@ElvishJerricco ElvishJerricco merged commit 0a3ed67 into NixOS:staging Aug 14, 2024
8 of 9 checks passed
@ElvishJerricco
Copy link
Contributor

Thanks @squalus! I did a squash merge just to get those comment tweaks all into the one commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failure: osquery-5.12.2
2 participants