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

pathname: new methods and improvements #3377

Merged
merged 1 commit into from Nov 10, 2017

Conversation

Projects
None yet
5 participants
@maxim-belkin
Contributor

maxim-belkin commented Oct 26, 2017

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

CC @sjackman

extend/pathname.rb

  • atomic_write: close file before renaming to prevent 'Device or resource busy' error
  • ensure_writable: preserve executable bit

extend/os/pathname.rb
extend/os/linux/extend/pathname.rb
- new elf? and dynamic? methods

Show outdated Hide outdated Library/Homebrew/extend/os/linux/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/pathname.rb Outdated
@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Oct 26, 2017

Contributor

Thanks, Maxim! I have a few style suggestions.

Contributor

sjackman commented Oct 26, 2017

Thanks, Maxim! I have a few style suggestions.

Show outdated Hide outdated Library/Homebrew/extend/os/linux/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/os/linux/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/pathname.rb Outdated
@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Oct 30, 2017

Contributor

One of the error messages https://travis-ci.org/Homebrew/brew/jobs/294948205#L945:

1) Pathname#ensure_writable makes a file writable and restores permissions afterwards
     Failure/Error:
       file.ensure_writable do
         expect(file).to be_writable
       end
     TypeError:
       no implicit conversion of String into Integer
Contributor

maxim-belkin commented Oct 30, 2017

One of the error messages https://travis-ci.org/Homebrew/brew/jobs/294948205#L945:

1) Pathname#ensure_writable makes a file writable and restores permissions afterwards
     Failure/Error:
       file.ensure_writable do
         expect(file).to be_writable
       end
     TypeError:
       no implicit conversion of String into Integer
@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Oct 30, 2017

Contributor
 no implicit conversion of String into Integer

As far as I can tell, FileUtils.chmod accepts a symbolic string mode, but Pathname.chmod does not and File.chmod does not. You can instead use…

chmod 0644 | saved_perms

or

FileUtils.chmod "u+rw", to_path
Contributor

sjackman commented Oct 30, 2017

 no implicit conversion of String into Integer

As far as I can tell, FileUtils.chmod accepts a symbolic string mode, but Pathname.chmod does not and File.chmod does not. You can instead use…

chmod 0644 | saved_perms

or

FileUtils.chmod "u+rw", to_path
@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Oct 31, 2017

Contributor

chmod 0644 | saved_perms is too cryptic. I'll try FileUtils.chmod "u+rw", to_path

Contributor

maxim-belkin commented Oct 31, 2017

chmod 0644 | saved_perms is too cryptic. I'll try FileUtils.chmod "u+rw", to_path

@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Oct 31, 2017

Contributor

codecov/patch shows reduced coverage but otherwise success

Contributor

maxim-belkin commented Oct 31, 2017

codecov/patch shows reduced coverage but otherwise success

Show outdated Hide outdated Library/Homebrew/extend/os/linux/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/os/linux/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/os/linux/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/os/linux/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/os/pathname.rb Outdated
@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Nov 1, 2017

Contributor

https://travis-ci.org/Homebrew/brew/jobs/295748996#L846

Offenses:
Library/Homebrew/extend/os/linux/extend/pathname.rb:10:5: C: Use a guard clause instead of wrapping the code inside a conditional expression.
    if !which("readelf").nil?
    ^^
Library/Homebrew/extend/os/linux/extend/pathname.rb:11:7: C: Redundant return detected.
      return popen_read("readelf", "-l", to_path).include?(" DYNAMIC ")
      ^^^^^^
Library/Homebrew/extend/os/linux/extend/pathname.rb:13:7: C: Redundant return detected.
      return !popen_read("file", "-L", "-b", to_path)[/dynamic|shared/].nil?
      ^^^^^^
663 files inspected, 3 offenses detected

So, I will change dynamic_elf? from:

  def dynamic_elf?
    if !which("readelf").nil?
      return popen_read("readelf", "-l", to_path).include?(" DYNAMIC ")
    elsif !which("file").nil?
      return !popen_read("file", "-L", "-b", to_path)[/dynamic|shared/].nil?
    else
      raise StandardError, "Neither `readelf` nor `file` is available to determine whether '#{self}' is dynamically or statically linked."
    end

to

  def dynamic_elf?
    return popen_read("readelf", "-l", to_path).include?(" DYNAMIC ") unless which("readelf").nil?
    return !popen_read("file", "-L", "-b", to_path)[/dynamic|shared/].nil? unless which("file").nil?
    raise StandardError, "Neither `readelf` nor `file` is available to determine whether '#{self}' is dynamically or statically linked."
  end 
Contributor

maxim-belkin commented Nov 1, 2017

https://travis-ci.org/Homebrew/brew/jobs/295748996#L846

Offenses:
Library/Homebrew/extend/os/linux/extend/pathname.rb:10:5: C: Use a guard clause instead of wrapping the code inside a conditional expression.
    if !which("readelf").nil?
    ^^
Library/Homebrew/extend/os/linux/extend/pathname.rb:11:7: C: Redundant return detected.
      return popen_read("readelf", "-l", to_path).include?(" DYNAMIC ")
      ^^^^^^
Library/Homebrew/extend/os/linux/extend/pathname.rb:13:7: C: Redundant return detected.
      return !popen_read("file", "-L", "-b", to_path)[/dynamic|shared/].nil?
      ^^^^^^
663 files inspected, 3 offenses detected

So, I will change dynamic_elf? from:

  def dynamic_elf?
    if !which("readelf").nil?
      return popen_read("readelf", "-l", to_path).include?(" DYNAMIC ")
    elsif !which("file").nil?
      return !popen_read("file", "-L", "-b", to_path)[/dynamic|shared/].nil?
    else
      raise StandardError, "Neither `readelf` nor `file` is available to determine whether '#{self}' is dynamically or statically linked."
    end

to

  def dynamic_elf?
    return popen_read("readelf", "-l", to_path).include?(" DYNAMIC ") unless which("readelf").nil?
    return !popen_read("file", "-L", "-b", to_path)[/dynamic|shared/].nil? unless which("file").nil?
    raise StandardError, "Neither `readelf` nor `file` is available to determine whether '#{self}' is dynamically or statically linked."
  end 
Show outdated Hide outdated Library/Homebrew/extend/os/linux/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/os/linux/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/extend/os/pathname.rb Outdated
@@ -1,5 +1,3 @@
require "os/mac/pathname"

This comment has been minimized.

@MikeMcQuaid
@MikeMcQuaid

MikeMcQuaid Nov 3, 2017

Member

?

This comment has been minimized.

@sjackman

sjackman Nov 3, 2017

Contributor

I think this line was an anachronism, before extend/os existed. My opinion…

  • extend/os/mac/hardware/cpu.rb should not require os/mac/pathname.rb
  • extend/pathname.rb should require extend/os/pathname.rb which requires extend/os/mac/extend/pathname.rb or extend/os/linux/extend/pathname.rb
@sjackman

sjackman Nov 3, 2017

Contributor

I think this line was an anachronism, before extend/os existed. My opinion…

  • extend/os/mac/hardware/cpu.rb should not require os/mac/pathname.rb
  • extend/pathname.rb should require extend/os/pathname.rb which requires extend/os/mac/extend/pathname.rb or extend/os/linux/extend/pathname.rb

This comment has been minimized.

@maxim-belkin

maxim-belkin Nov 4, 2017

Contributor

Question: is there a way to detect anachronisms like this? That is, something is "required" but not, in fact, necessary.

@maxim-belkin

maxim-belkin Nov 4, 2017

Contributor

Question: is there a way to detect anachronisms like this? That is, something is "required" but not, in fact, necessary.

Show outdated Hide outdated Library/Homebrew/extend/pathname.rb Outdated
Show outdated Hide outdated Library/Homebrew/os/mac.rb Outdated
@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Nov 4, 2017

Contributor

OK, here is the current dependency "diagram":

  • extend/pathname requires extend/os/pathname
  • extend/os/pathname requires extend/os/<OS>/extend/pathname
Contributor

maxim-belkin commented Nov 4, 2017

OK, here is the current dependency "diagram":

  • extend/pathname requires extend/os/pathname
  • extend/os/pathname requires extend/os/<OS>/extend/pathname
pathname: improvements, cleanups, and new methods
- atomic_write: close file before renaming to prevent error:
  'Device or resource busy'
- ensure_writable: preserve executable bit
- new elf? and dynamic? methods
@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Nov 8, 2017

Contributor

@sjackman @MikeMcQuaid
is there anything else you guys want me to do/change in this PR?

Contributor

maxim-belkin commented Nov 8, 2017

@sjackman @MikeMcQuaid
is there anything else you guys want me to do/change in this PR?

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Nov 10, 2017

Member

Nice work here, thanks again @maxim-belkin!

Member

MikeMcQuaid commented Nov 10, 2017

Nice work here, thanks again @maxim-belkin!

@MikeMcQuaid MikeMcQuaid merged commit ee41721 into Homebrew:master Nov 10, 2017

1 of 3 checks passed

codecov/patch 22.72% of diff hit (target 69.17%)
Details
codecov/project 69.12% (-0.06%) compared to ccf933f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Nov 10, 2017

Contributor

Thanks, Maxim! and thanks Mike for feedback and for merging. 👏

Contributor

sjackman commented Nov 10, 2017

Thanks, Maxim! and thanks Mike for feedback and for merging. 👏

@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Nov 17, 2017

Contributor

yay!

Contributor

maxim-belkin commented Nov 17, 2017

yay!

@maxim-belkin maxim-belkin deleted the maxim-belkin:extend-pathname branch Nov 17, 2017

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