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

enable patchelf.rb writing for devs. #8524

Merged
merged 1 commit into from Aug 28, 2020

Conversation

rmNULL
Copy link
Contributor

@rmNULL rmNULL commented Aug 28, 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?

cc @sjackman @woodruffw

Comment on lines 5 to 6
ENV["HOMEBREW_PATCHELF_RB_WRITE"].present? || ENV["HOMEBREW_DEVELOPER"].present? &&
!(ENV["CI"].present? || ENV["NO_HOMEBREW_PATCHELF_RB_WRITE"].present?)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ENV["HOMEBREW_PATCHELF_RB_WRITE"].present? || ENV["HOMEBREW_DEVELOPER"].present? &&
!(ENV["CI"].present? || ENV["NO_HOMEBREW_PATCHELF_RB_WRITE"].present?)
ENV["HOMEBREW_PATCHELF_RB_WRITE"].present?
|| ENV["HOMEBREW_DEVELOPER"].present?
&& !ENV["CI"].present?
&& !ENV["HOMEBREW_NO_PATCHELF_RB_WRITE"].present?)

Copy link
Contributor Author

@rmNULL rmNULL Aug 28, 2020

Choose a reason for hiding this comment

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

i'll separate the &s.
ruby has problems parsing operators on next line,
even i prefer
A
&& b
|| c

over
A &&
b ||
c

P.S: using backslash is awkward -_-

Comment on lines 5 to 6
ENV["HOMEBREW_PATCHELF_RB_WRITE"].present? ||
ENV["HOMEBREW_DEVELOPER"].present? &&
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ENV["HOMEBREW_PATCHELF_RB_WRITE"].present? ||
ENV["HOMEBREW_DEVELOPER"].present? &&
(ENV["HOMEBREW_PATCHELF_RB_WRITE"].present? ||
ENV["HOMEBREW_DEVELOPER"].present?) &&

Comment on lines 5 to 8
(ENV["HOMEBREW_PATCHELF_RB_WRITE"].present? ||
ENV["HOMEBREW_DEVELOPER"].present?) &&
!ENV["CI"].present? &&
!ENV["NO_HOMEBREW_PATCHELF_RB_WRITE"].present?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(ENV["HOMEBREW_PATCHELF_RB_WRITE"].present? ||
ENV["HOMEBREW_DEVELOPER"].present?) &&
!ENV["CI"].present? &&
!ENV["NO_HOMEBREW_PATCHELF_RB_WRITE"].present?
!ENV["HOMEBREW_NO_PATCHELF_RB_WRITE"].present? &&
(ENV["HOMEBREW_PATCHELF_RB_WRITE"].present? ||
(!ENV["CI"].present? && ENV["HOMEBREW_DEVELOPER"].present?))

if HOMEBREW_NO_PATCHELF_RB_WRITE then off
if HOMEBREW_PATCHELF_RB_WRITE then on
if CI then off
if HOMEBREW_DEVELOPER then on
otherwise off

@sjackman sjackman self-assigned this Aug 28, 2020
@sjackman sjackman merged commit 9bbd866 into Homebrew:master Aug 28, 2020
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 14, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants