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

use system rustup to get rustfmt #2055

Closed
wants to merge 2 commits into from
Closed

use system rustup to get rustfmt #2055

wants to merge 2 commits into from

Conversation

joske
Copy link
Contributor

@joske joske commented Jul 4, 2023

A first stab at creating a new package.yaml for rustfmt as the current one is broken (see #2054).

I could not figure out how I can test this.

Please comment on how it could be improved.

@williambotman williambotman requested a review from a team July 4, 2023 08:19
Copy link
Member

@williamboman williamboman left a comment

Choose a reason for hiding this comment

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

I opened an issue with them regarding this recently, and it seems unlikely that the release binaries they provide will be functional going forward. As mentioned in #2054, I think the appropriate action would be to deprecate rustfmt in Mason and inform users to install and manage it externally via rustup component. This is also the guidance from rustfmt maintainers, and is the only officially supported installation method.

Comment on lines +14 to +17
id: pkg:generic/rustfmt@x
build:
run: cp $(which rustfmt) . || echo "rustfmt not found"
bin: rustfmt
Copy link
Member

@williamboman williamboman Jul 4, 2023

Choose a reason for hiding this comment

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

This is problematic for a couple of reasons:

  1. It has a placeholder version number. Packages must refer to real version numbers.
  2. The build script simply copies an external installation of rustfmt into the Mason installation directory. This is not a self-contained installation, and may also be problematic for loading linked libraries (relative rpath). This is likely also problematic because it will use whatever toolchain is currently active.
  3. Doesn't support Windows (although this would be an easy fix by providing a powershell script as well).

@williamboman
Copy link
Member

Superseded by #2059

@joske
Copy link
Contributor Author

joske commented Jul 4, 2023

  1. We could easily put the version number of the rustfmt release and we could track that
  2. I tried this locally and just copying the rustfmt binary into mason works (macOS)
  3. I don't know anything about powershell unfortunately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants