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.

Sign up for GitHub

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

Jump to bottom

Cleaner: ELF files are executable on Linux #3469

Merged
merged 2 commits into from
Nov 27, 2017
Merged

Cleaner: ELF files are executable on Linux #3469

merged 2 commits into from
Nov 27, 2017

Conversation

sjackman
Copy link
Member

  • 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?

@MikeMcQuaid
Copy link
Member

Breaking some Linux tests: https://travis-ci.org/Homebrew/brew/jobs/305958848

@sjackman
Copy link
Member Author

sjackman commented Nov 22, 2017 •
edited
Loading

The binary files in test/support/fixtures/tarballs/testball-0.1.tbz are Mach-O executables.
I've added testball-0.1-linux.tbz with ELF executables.
Should I rename testball-0.1.tbz to testball-0.1-mac.tbz?

@MikeMcQuaid
Copy link
Member

brew tests still failing here but the approach in general looks sound.

@sjackman sjackman force-pushed the cleaner branch 2 times, most recently from fd52a7b to 6ca2a0d Compare November 24, 2017 00:30
@sjackman
Copy link
Member Author

sjackman commented Nov 24, 2017 •
edited
Loading

I've fixed the test failures.

MikeMcQuaid
MikeMcQuaid requested changes Nov 24, 2017
View reviewed changes
Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb Outdated
url "file://#{TEST_FIXTURE_DIR}/tarballs/testball-0.1.tbz"
sha256 "#{TESTBALL_SHA256}"
url "file://#{TEST_FIXTURE_DIR}/tarballs/#{OS.linux? ? "testball-0.1-linux.tbz" : "testball-0.1.tbz"}"
sha256 "#{OS.linux? ? TESTBALL_SHA256_LINUX : TESTBALL_SHA256}"
Copy link
Member

Choose a reason for hiding this comment

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

Can this use the same approach as above?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. This code defines the location of the tarball and its checksum, which is platform dependent.

Formulary.factory("testball1").stable.checksum.hexdigest

Copy link
Member Author

Choose a reason for hiding this comment

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

It could calculate the checksum from the contents of the file, rather than store it in Ruby code.

Copy link
Member

Choose a reason for hiding this comment

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

It could calculate the checksum from the contents of the file, rather than store it in Ruby code.

Yes, let's do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Library/Homebrew/test/cleaner_spec.rb Outdated
@@ -20,7 +20,8 @@

subject.clean

expect((f.bin/"a.out").stat.mode).to eq(0100555)
mach_executable_perm = OS.linux? ? 0100444 : 0100555
Copy link
Member

Choose a reason for hiding this comment

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

Could we set these to the same in the tarball instead?

Copy link
Member Author

@sjackman sjackman Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The file permissions in the tarball are the same. This test is related to a file outside of the tarball,test/support/fixtures/mach/a.out is a Mach-O file. On Linux, Cleaner::executable_path? changes ELF files and shell scripts to executable, but not Mach-O files. I can create a separate test/support/fixtures/elf directory for Linux. Alternatively, I can disable this particular test on Linux using :needs_macos.

Copy link
Member

Choose a reason for hiding this comment

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

I can create a separate test/support/fixtures/elf directory for Linux.

Sounds good 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@sjackman sjackman force-pushed the cleaner branch 2 times, most recently from 10d6cc9 to 744631c Compare November 25, 2017 19:06
MikeMcQuaid
MikeMcQuaid reviewed Nov 26, 2017
View reviewed changes
Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb Outdated
@@ -123,11 +123,12 @@ def brew(*args)
def setup_test_formula(name, content = nil)
case name
when /^testball/
tarball = TEST_FIXTURE_DIR/"tarballs/#{OS.linux? ? "testball-0.1-linux.tbz" : "testball-0.1.tbz"}"
Copy link
Member

Choose a reason for hiding this comment

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

Can you use an if/else here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing.

@sjackman
Add test fixtures for ELF executables
3cd8696 
Add test/support/fixtures/elf/
and test/support/fixtures/tarballs/testball-0.1-linux.tbz
@MikeMcQuaid MikeMcQuaid merged commit 323aaa0 into Homebrew:master Nov 27, 2017
@MikeMcQuaid
Copy link
Member

Thanks again @sjackman!

@sjackman sjackman deleted the cleaner branch November 27, 2017 16:16
@sjackman
Copy link
Member Author

Thanks again for merging, Mike!

@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.
Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sjackman @MikeMcQuaid