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

Refactoring keg_relocate to use ELFShim#interpreter #7797

Merged
merged 1 commit into from Jun 26, 2020

Conversation

rmNULL
Copy link
Contributor

@rmNULL rmNULL commented Jun 22, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Replaces all uses of ELFShim#with_interpreter? and removes it.
Introduce ELFShim#interpreter to return the interpreter in path
if present otherwise return nil.

@sjackman @woodruffw @MikeMcQuaid
there are style issues and concerns parsing interpreter from the output of shell command,
these are discussed in review comments.

Library/Homebrew/extend/os/linux/keg_relocate.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/elf.rb Outdated Show resolved Hide resolved
Library/Homebrew/extend/os/linux/keg_relocate.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/elf.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/elf.rb Outdated Show resolved Hide resolved
@rmNULL rmNULL force-pushed the interpreter-method-for-elfshim branch from 72e437f to f9234ad Compare June 23, 2020 01:34
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM!

Library/Homebrew/extend/os/linux/keg_relocate.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/elf.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/elf.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/elf.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/elf.rb Outdated Show resolved Hide resolved
@sjackman
Copy link
Member

This PR drops support for getting the interpreter using readelf. I'm okay with that though.

$ readelf -l /bin/ls
…
  INTERP         0x0000000000000238 0x0000000000400238 0x0000000000400238
                 0x000000000000001c 0x000000000000001c  R      1
      [Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]

@rmNULL rmNULL requested a review from sjackman June 23, 2020 03:03
@rmNULL
Copy link
Contributor Author

rmNULL commented Jun 23, 2020

would we need tests for this method ?

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

This PR drops support for getting the interpreter using readelf. I'm okay with that though.

How often is this being used by users? Can we figure this out and how disruptive it'll be before removing the code? There's not much code there and it seems weird to drop it unless it's unused/causing problems.

Copy link
Member

@sjackman sjackman left a comment

Choose a reason for hiding this comment

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

Nearly there!

Library/Homebrew/extend/os/linux/keg_relocate.rb Outdated Show resolved Hide resolved
Library/Homebrew/extend/os/linux/keg_relocate.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/elf.rb Outdated Show resolved Hide resolved
@sjackman
Copy link
Member

sjackman commented Jun 23, 2020

This PR drops support for getting the interpreter using readelf. I'm okay with that though.

How often is this being used by users? Can we figure this out and how disruptive it'll be before removing the code? There's not much code there and it seems weird to drop it unless it's unused/causing problems.

I believe this code is never currently used, because this interpreter method is called only by keg_relocate, which guarantees that patchelf is installed, and so patchelf will be used instead of readelf. That said, it's easy enough to implement readelf, so may as well.

@rmNULL Parse this line using a regex.

$ readelf -l /bin/ls
…
 [Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]
…

@rmNULL
Copy link
Contributor Author

rmNULL commented Jun 23, 2020

This PR drops support for getting the interpreter using readelf. I'm okay with that though.

How often is this being used by users? Can we figure this out and how disruptive it'll be before removing the code? There's not much code there and it seems weird to drop it unless it's unused/causing problems.

@MikeMcQuaid
there was no much code because it was just checking for presence of interpreter section.
however here, we have to parse that text from the output, which is a little arguable more code.

Another case, (as of now) this method is called only once(throughout the whole source),
and patchelf is guaranteed to exist when that call is made.
patchelf was a requirement before the change too, in that way we are not dropping any support.

@rmNULL rmNULL force-pushed the interpreter-method-for-elfshim branch from 69b46a8 to cc45214 Compare June 24, 2020 01:22
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Another case, (as of now) this method is called only once(throughout the whole source),
and patchelf is guaranteed to exist when that call is made.
patchelf was a requirement before the change too, in that way we are not dropping any support.

Ok, fine with me to remove that code!

@rmNULL rmNULL force-pushed the interpreter-method-for-elfshim branch from 51f4915 to e43a56b Compare June 25, 2020 00:38
@rmNULL rmNULL marked this pull request as draft June 25, 2020 00:56
@rmNULL rmNULL force-pushed the interpreter-method-for-elfshim branch from e43a56b to ca32011 Compare June 25, 2020 01:18
@rmNULL rmNULL marked this pull request as ready for review June 25, 2020 01:42
@rmNULL rmNULL requested a review from sjackman June 25, 2020 01:42
@rmNULL rmNULL force-pushed the interpreter-method-for-elfshim branch from ca32011 to 35328ed Compare June 26, 2020 00:11
@woodruffw woodruffw changed the title [draft] refactoring keg_relocate to use ELFShim#interpreter Refactoring keg_relocate to use ELFShim#interpreter Jun 26, 2020
@woodruffw woodruffw merged commit b725d65 into Homebrew:master Jun 26, 2020
@woodruffw
Copy link
Member

Thanks, @rmNULL 🎉

@rmNULL rmNULL deleted the interpreter-method-for-elfshim branch June 27, 2020 02:55
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 27, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement linux outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants