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

Implement linkage for Linux #3517

Merged
merged 2 commits into from Dec 4, 2017
Merged

Implement linkage for Linux #3517

merged 2 commits into from Dec 4, 2017

Conversation

sjackman
Copy link
Member

@sjackman sjackman commented Dec 2, 2017

extend/pathname: Add os/linux/elf.rb

  • 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 tests with your changes locally?

return @elf = false unless read(4) == "\x7fELF"
# Check that the ELF file is for Linux.
@elf = case read(1, 7).unpack("C")[0]
when 0, 3 then true
Copy link
Member

Choose a reason for hiding this comment

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

The above two lines aren't particularly obvious in what they are doing. Can you add a comment and then we can hopefully figure out how to make them more readable?

def elf?
return @elf if defined? @elf
# See: https://en.wikipedia.org/wiki/Executable_and_Linkable_Format#File_header
return @elf = false unless read(4) == "\x7fELF"
Copy link
Member

Choose a reason for hiding this comment

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

I'd say pretty much every number in this file should be a constant explaining what it means otherwise they are a bit "magic".

elsif which "file"
!Utils.popen_read("file", "-L", "-b", to_path)[/dynamic|shared/].nil?
else
raise StandardError, "Neither `readelf` nor `file` is available "\
Copy link
Member

Choose a reason for hiding this comment

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

Is StandardError not the default for raise? I can't remember. Also, make this message no more than 80 characters or use <<~EOS`

end

class Metadata
LDD_RX = /\t.* => (.*) \(.*\)|\t(.*) => not found/
Copy link
Member

Choose a reason for hiding this comment

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

Don't know what LDD_RX means

@dylib_id, needed = needed_libraries path
return if needed.empty?
# ldd requires that the file be executable, and all ELF files should be executable.
FileUtils.chmod "+x", path unless path.executable?
Copy link
Member

Choose a reason for hiding this comment

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

A Metadata.new shouldn't be modifying files.

lines.each do |s|
case s
when /\(SONAME\)/
soname = s[/\[(.*)\]/, 1]
Copy link
Member

Choose a reason for hiding this comment

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

This regex is repeated so consider making it a constant

raise ErrorDuringExecution, command unless $CHILD_STATUS.success?
lines.each do |s|
case s
when /\(SONAME\)/
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be a regex.

case s
when /\(SONAME\)/
soname = s[/\[(.*)\]/, 1]
when /\(NEEDED\)/
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be a regex.

metadata.dylib_id
end

def dynamically_linked_libraries(**_options)
Copy link
Member

Choose a reason for hiding this comment

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

Can probably just make this (*)

@sjackman sjackman force-pushed the linkage branch 2 times, most recently from 40668f6 to 1af0516 Compare December 3, 2017 07:32
@sjackman
Copy link
Member Author

sjackman commented Dec 3, 2017

Thanks for the feedback, Mike. I've addressed your comments.

@sjackman sjackman force-pushed the linkage branch 3 times, most recently from 69baaf6 to 4cfe957 Compare December 3, 2017 08:09
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.

Looking much better! A few more nits, thanks.

@@ -0,0 +1,151 @@
module ELFShim
# See: https://en.wikipedia.org/wiki/Executable_and_Linkable_Format#File_header
MAGIC_OFFSET = 0
Copy link
Member

Choose a reason for hiding this comment

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

MAGIC_NUMBER_OFFSET

module ELFShim
# See: https://en.wikipedia.org/wiki/Executable_and_Linkable_Format#File_header
MAGIC_OFFSET = 0
MAGIC_SIZE = 4
Copy link
Member

Choose a reason for hiding this comment

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

Use MAGIC_NUMBER.size

MAGIC_SIZE = 4
MAGIC_NUMBER = "\x7fELF".freeze

OSABI_OFFSET = 0x07
Copy link
Member

Choose a reason for hiding this comment

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

OS_ABI_OFFSET

MAGIC_NUMBER = "\x7fELF".freeze

OSABI_OFFSET = 0x07
OSABI_SIZE = 1
Copy link
Member

Choose a reason for hiding this comment

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

Use OSABI_LINUX.to_s.size

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a read_uint8 helper function.

OSABI_LINUX = 3

TYPE_OFFSET = 0x10
TYPE_SIZE = 2
Copy link
Member

Choose a reason for hiding this comment

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

Use TYPE_EXECUTABLE.to_s.size

Copy link
Member Author

Choose a reason for hiding this comment

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

These are not equivalent.
TYPE_EXECUTABLE.to_s.sizeTYPE_SIZE
TYPE_EXECUTABLE.to_s.size = 1
TYPE_SIZE = 2

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a read_uint16 helper function.

return @elf = false unless read(MAGIC_SIZE, MAGIC_OFFSET) == MAGIC_NUMBER
# Check that this ELF file is for Linux or System V.
# OSABI is often set to 0 (System V), regardless of the target platform.
osabi = read(OSABI_SIZE, OSABI_OFFSET).unpack("C")[0]
Copy link
Member

Choose a reason for hiding this comment

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

.first rather than [0]

Copy link
Member

Choose a reason for hiding this comment

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

os_abi


def arch
return :dunno unless elf?
@arch ||= case read(MACHINE_SIZE, MACHINE_OFFSET).unpack("v")[0]
Copy link
Member

Choose a reason for hiding this comment

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

.first rather than [0]

# Check that this ELF file is for Linux or System V.
# OSABI is often set to 0 (System V), regardless of the target platform.
osabi = read(OSABI_SIZE, OSABI_OFFSET).unpack("C")[0]
@elf = osabi == OSABI_LINUX || osabi == OSABI_SYSTEM_V
Copy link
Member

Choose a reason for hiding this comment

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

[OSABI_SYSTEM_V, OSABI_LINUX].include?(osabi)


def elf_type
return :dunno unless elf?
@elf_type ||= case read(TYPE_SIZE, TYPE_OFFSET).unpack("v")[0]
Copy link
Member

Choose a reason for hiding this comment

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

.first rather than [0]

@path = path
@dylibs = []
@dylib_id, needed = needed_libraries path
return if needed.empty?
Copy link
Member

Choose a reason for hiding this comment

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

Stick a newline after each return; makes this easier to follow/scan (and consider doing this elsewhere in the file).

@sjackman
Copy link
Member Author

sjackman commented Dec 4, 2017

Fixed. Thanks again for the feedback, Mike.

@MikeMcQuaid MikeMcQuaid merged commit 4f5e938 into Homebrew:master Dec 4, 2017
@MikeMcQuaid
Copy link
Member

Thanks again @sjackman!

@MikeMcQuaid
Copy link
Member

@sjackman Merging this because there's been a lot of back and forth but ideally we'd get some tests to cover a bit more of the diff, here. Can do that in follow-up PR.

@sjackman sjackman deleted the linkage branch December 4, 2017 17:40
@sjackman
Copy link
Member Author

sjackman commented Dec 4, 2017

It looks as though Codecov doesn't count lines-of-code as covered that are covered by tests that run only on Linux. Is that the case?
https://codecov.io/gh/Homebrew/brew/compare/0ad42ebbcb735422612a7c6d4edc5ad88044dd4b...d79c5ade1abaad0fb1ea88f2b949def8dcfddb81/diff#diff-TGlicmFyeS9Ib21lYnJldy9vcy9saW51eC9lbGYucmI=

@sjackman
Copy link
Member Author

sjackman commented Dec 4, 2017

Thanks for merging, Mike!

@MikeMcQuaid
Copy link
Member

It looks as though Codecov doesn't count lines-of-code as covered that are covered by tests that run only on Linux. Is that the case?

@sjackman Correct. If we can figure out a way to upload and combine the coverage that would be good. @reitermarkus looked at this before I think; any thoughts Markus?

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants