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

rustfmt 1.6.0 #137033

Closed
wants to merge 1 commit into from
Closed

rustfmt 1.6.0 #137033

wants to merge 1 commit into from

Conversation

henryhchchc
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

The following changes are made:

@github-actions github-actions bot added the rust Rust use is a significant feature of the PR or issue label Jul 19, 2023
@henryhchchc henryhchchc force-pushed the rustfmt-1.6.0 branch 6 times, most recently from 6043e57 to 28add7d Compare July 20, 2023 00:25
@chenrui333 chenrui333 added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Jul 20, 2023
@henryhchchc henryhchchc force-pushed the rustfmt-1.6.0 branch 5 times, most recently from 80bc9f4 to 48543be Compare July 20, 2023 02:30
This was referenced Jul 20, 2023
Closed
Closed
branchvincent
branchvincent reviewed Jul 20, 2023
View reviewed changes
Formula/rustfmt.rb Outdated Show resolved Hide resolved
branchvincent
branchvincent reviewed Jul 20, 2023
View reviewed changes
Formula/rustfmt.rb Outdated Show resolved Hide resolved
carlocab
carlocab reviewed Jul 20, 2023
View reviewed changes
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

I think rustfmt can be built alongside rust, so I'm wondering if it makes more sense to just bundle this with the rust formula.

Formula/rustfmt.rb Outdated
Comment on lines 25 to 27
on_linux do
depends_on "patchelf" => :build
end
Copy link
Member

Choose a reason for hiding this comment

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

patchelf.rb should be available via brew, so we should try to use that instead of depending on patchelf. CC @Homebrew/linux for suggestions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the patchelf.rb formula or the API?

Formula/rustfmt.rb Outdated Show resolved Hide resolved
Formula/rustfmt.rb Outdated Show resolved Hide resolved
Formula/rustfmt.rb Outdated
Comment on lines 57 to 58
system "patchelf", *patch_libs, "#{bin}/rustfmt"
system "patchelf", *patch_libs, "#{bin}/git-rustfmt"
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 try using patchelf.rb here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched to that API and seems like it does not work. Is there any example I can refer to?

@henryhchchc henryhchchc force-pushed the rustfmt-1.6.0 branch 7 times, most recently from 70414a7 to fe4369d Compare July 20, 2023 08:32
@henryhchchc henryhchchc force-pushed the rustfmt-1.6.0 branch 6 times, most recently from 3843083 to 0bf24e3 Compare July 20, 2023 11:17
@ZhongRuoyu
Copy link
Member

I think rustfmt can be built alongside rust, so I'm wondering if it makes more sense to just bundle this with the rust formula.

It seems that this is already the case in #136514: https://github.com/Homebrew/homebrew-core/actions/runs/5594719599/jobs/10233272477?pr=136514#step:4:202

@henryhchchc
Copy link
Contributor Author

I think rustfmt can be built alongside rust, so I'm wondering if it makes more sense to just bundle this with the rust formula.

It seems that this is already the case in #136514: https://github.com/Homebrew/homebrew-core/actions/runs/5594719599/jobs/10233272477?pr=136514#step:4:202

That's cool. If so would we consider closing this PR for now?

@chenrui333
Copy link
Member

rust-analyzer is in the same boat, and probably need the similar handling as in here

@chenrui333 chenrui333 removed their request for review August 9, 2023 12:32
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Aug 31, 2023
@henryhchchc
Copy link
Contributor Author

Let's close it for now.

@henryhchchc henryhchchc deleted the rustfmt-1.6.0 branch August 31, 2023 02:23
@ZhongRuoyu ZhongRuoyu mentioned this pull request Aug 31, 2023
Merged
6 tasks
@ZhongRuoyu
Copy link
Member

@henryhchchc, thanks for your work. We've shipped Rust 1.72.0 in #140367, which unbundled rust-analyzer and rustfmt, so I'm continuing your work in #141018.

@github-actions github-actions bot added the outdated PR was locked due to age label Oct 1, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chenrui333 chenrui333 chenrui333 left review comments

@branchvincent branchvincent branchvincent left review comments

@carlocab carlocab carlocab left review comments

Assignees
No one assigned
Labels
CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. outdated PR was locked due to age rust Rust use is a significant feature of the PR or issue stale No recent activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@henryhchchc @ZhongRuoyu @chenrui333 @branchvincent @carlocab