Skip to content

text_cop: require cargo to use install instead of build#4311

Merged
ilovezfs merged 1 commit intoHomebrew:masterfrom
commitay:cargo-install
Jun 10, 2018
Merged

text_cop: require cargo to use install instead of build#4311
ilovezfs merged 1 commit intoHomebrew:masterfrom
commitay:cargo-install

Conversation

@commitay
Copy link
Copy Markdown
Contributor

@commitay commitay commented Jun 9, 2018

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

Opened this for discussion, I'm still checking if all of the build formulae work as expected with install or if we would need to whitelist any.

Currently we prefer cargo install --root prefix (which doesn't need a bin.install) over cargo build. We have 27 formulae that run cargo, 18 have build and 9 have install.

@ghost ghost assigned commitay Jun 9, 2018
@ghost ghost added the in progress Maintainers are working on this label Jun 9, 2018
@ilovezfs
Copy link
Copy Markdown
Contributor

ilovezfs commented Jun 9, 2018

I don't think we can rule out the possibility of legitimately needing to run cargo build not cargo install.

Comment thread Library/Homebrew/rubocops/text_cop.rb Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--root

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@commitay
Copy link
Copy Markdown
Contributor Author

I don't think we can rule out the possibility of legitimately needing to run cargo build not cargo install.

Yeah, I guess it's possible. I haven't found any formulae that need build yet but I'm sure something will come along. Mostly wanted this to nudge new formulae to the preferred style but I suppose we could change all the formulae that we can to this style and hope people copy the right one?

@ilovezfs
Copy link
Copy Markdown
Contributor

It looks like they're equivalent except that install defaults to a --release build whereas build defaults to a --debug build so I think we'll be OK.

@commitay
Copy link
Copy Markdown
Contributor Author

Do you want PRs changing formulae to use install or just wait til they get bumped and fail audit?

@ilovezfs
Copy link
Copy Markdown
Contributor

Voilà Homebrew/homebrew-core#28846

@ilovezfs ilovezfs merged commit 00e3024 into Homebrew:master Jun 10, 2018
@ghost ghost removed the in progress Maintainers are working on this label Jun 10, 2018
@commitay commitay deleted the cargo-install branch June 10, 2018 13:45
@commitay commitay removed their assignment Jun 10, 2018
@ilovezfs
Copy link
Copy Markdown
Contributor

Thanks @commitay!

@lock lock bot added the outdated PR was locked due to age label Jul 10, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants