Rust as 1st class language by Holzhaus · Pull Request #2534 · pre-commit/pre-commit

Holzhaus · 2022-09-30T13:54:12Z

This adds support for pinning a specific rust version and bootstrapping rust with rustup/rustup-init.

I tried to follow the desired behavior described here: #1863 (comment)

Holzhaus · 2022-10-02T21:43:36Z

@asottile Unsure how to remove cargo/rust from the CI env to test the bootstrapping code. Maybe these tests can be added in a separate PR?

asottile · 2022-10-03T19:00:46Z

@asottile Unsure how to remove cargo/rust from the CI env to test the bootstrapping code. Maybe these tests can be added in a separate PR?

I think you can probably take the same approach that the other 1st-party languages use? where they stub out the "get default" to return None and observe the results

asottile · 2022-10-03T19:02:17Z

pre_commit/languages/rust.py

+                if sys.platform == 'win32':
+                    if platform.machine() == 'x86_64':
+                        url = 'https://win.rustup.rs/x86_64'
+                    else:
+                        url = 'https://win.rustup.rs/i686'
+                else:


I think windows can also run on arm64 -- might need to make a mapping of architectures and platforms to select the url out of that

I'm not a windows expert, but IIUC Windows on ARM has x86 emulation built in: https://learn.microsoft.com/en-us/windows/arm/overview#build-windows-apps-that-run-on-arm

asottile · 2022-10-03T19:03:37Z

pre_commit/languages/rust.py

+            toolchain = os.getenv('RUSTUP_TOOLCHAIN')
+            if toolchain is not None:
+                install_rust_with_toolchain(toolchain)


rather than ping-ponging through environment variables as globals this should just check directly -- I believe that'd be language_version != 'system'

We still need the toolchain type, but you're right that we don't need to get it from the env. See 7c22138.

pre_commit/languages/rust.py

asottile · 2022-10-03T21:52:38Z

anyway -- wanted to say I'm pretty excited for this PR :D

asottile · 2022-10-04T14:08:33Z

pre_commit/languages/rust.py

-                cwd=prefix.prefix_dir,
-            )
+        with in_env(prefix, version):
+            toolchain = TOOLCHAIN_FROM_VERSION.get(version, version)


I don't think TOOLCHAIN_FROM_VERSION is all that useful -- this condition here is just if version != 'system': rather than needing to go through this mapping

I only added the mapping to have a single source of truth for mapping C.DEFAULT to stable. I think I incorporated the changes in 8d545ad

pre_commit/languages/rust.py

tests/languages/rust_test.py

asottile · 2022-10-04T14:15:15Z

tests/languages/rust_test.py

+        toml.dumps({
+            'package': {
+                'name': 'hello_world',
+                'version': '0.1.0',
+                'edition': '2021',
+            },
+            'dependencies': {},
+        }),


can probably skip toml and just write a string here

I adapted this from the node tests which use json.dumps() for the package.json instead of writing a string directly. And we have a dependency on toml for reading the Cargo.toml file anyway. But I can change it if you want.

Holzhaus · 2022-10-04T16:38:40Z

The windows tests are failing because Windows has problems with long paths:

E           pre_commit.util.CalledProcessError: command: ('C:\\Users\\VssAdministrator\\.cargo\\bin\\cargo.EXE', 'install', '--bins', '--root', 'C:\\Users\\VssAdministrator\\AppData\\Local\\Temp\\pytest-of-unknown\\pytest-0\\test_local_rust_additional_dep0\\0\\.pre-commit\\repo04ryo5oe\\rustenv-system', 'hello-cli', '--version', '0.2.2')
E           return code: 101
E           expected return code: 0
E           stdout: (none)
E           stderr:
E               error: failed to initialize index git repository (in "C:\\Users\\VssAdministrator\\AppData\\Local\\Temp\\pytest-of-unknown\\pytest-0\\test_local_rust_additional_dep0\\0\\.pre-commit\\repo04ryo5oe\\rustenv-system\\registry\\index\\github.com-1ecc6299db9ec823")
E               
E               Caused by:
E                 path too long: 'C:/Users/VssAdministrator/AppData/Local/Temp/pytest-of-unknown/pytest-0/test_local_rust_additional_dep0/0/.pre-commit/repo04ryo5oe/rustenv-system/registry/index/github.com-1ecc6299db9ec823/.git/'; class=Filesystem (30)

Wouldn't have expected that a path of this length is an issue in 2022, but here we are now. Upstream issue seems to be rust-lang/cargo#9770. Ideas?

asottile · 2022-10-04T17:19:15Z

The windows tests are failing because Windows has problems with long paths:

E           pre_commit.util.CalledProcessError: command: ('C:\\Users\\VssAdministrator\\.cargo\\bin\\cargo.EXE', 'install', '--bins', '--root', 'C:\\Users\\VssAdministrator\\AppData\\Local\\Temp\\pytest-of-unknown\\pytest-0\\test_local_rust_additional_dep0\\0\\.pre-commit\\repo04ryo5oe\\rustenv-system', 'hello-cli', '--version', '0.2.2')
E           return code: 101
E           expected return code: 0
E           stdout: (none)
E           stderr:
E               error: failed to initialize index git repository (in "C:\\Users\\VssAdministrator\\AppData\\Local\\Temp\\pytest-of-unknown\\pytest-0\\test_local_rust_additional_dep0\\0\\.pre-commit\\repo04ryo5oe\\rustenv-system\\registry\\index\\github.com-1ecc6299db9ec823")
E               
E               Caused by:
E                 path too long: 'C:/Users/VssAdministrator/AppData/Local/Temp/pytest-of-unknown/pytest-0/test_local_rust_additional_dep0/0/.pre-commit/repo04ryo5oe/rustenv-system/registry/index/github.com-1ecc6299db9ec823/.git/'; class=Filesystem (30)

Wouldn't have expected that a path of this length is an issue in 2022, but here we are now. Upstream issue seems to be rust-lang/cargo#9770. Ideas?

might be able to use a shorter temporary directory -- we had TEMP: 'C:\temp' for a while for the windows tests -- not sure if that's still there or not or if we should bring it back

Holzhaus · 2022-10-04T20:55:36Z

I tried to shorten the path passing tox' envtmpdir as pytest --basetemp. This fixes the path length issue. Unfortunately, 3 other tests unrelated to to rust fail now :(

asottile · 2022-10-05T12:38:28Z

tox.ini

 [tox]
-envlist = py37,py38,pypy3,pre-commit
+# Platform specification support is available since version 2.0, see:
+# https://tox.wiki/en/latest/example/platform.html#basic-multi-platform-example
+minversion = 2.0
+envlist = {py37,py38,pypy3}-{unix,windows},pre-commit

 [testenv]
 deps = -rrequirements-dev.txt
 passenv = APPDATA HOME LOCALAPPDATA PROGRAMFILES RUSTUP_HOME
+platform =
+    unix: darwin|cygwin|linux\d*|aix\d*|sunos\d*|freebsd\d*
+    windows: win32
 commands =
-    coverage erase
-    coverage run -m pytest {posargs:tests}
-    coverage report
+    unix: coverage erase
+          coverage run -m pytest --basetemp="/tmp/pytest-{envname}" {posargs:tests}
+          coverage report
+    windows: coverage erase
+             coverage run -m pytest --basetemp="C:\tmp\pytest-{envname}" {posargs:tests}
+             coverage report

 [testenv:pre-commit]
 skip_install = true


I'd rather not change this -- it's intentionally simple. I meant to just add the temp override in azure-pipelines.yml since that's really the only place the paths are going to be so deep

Sorry, I have zero experience with Azure pipelines and I cannot really spend more time on this. You should have push acess to my branch. I'll remove the commit that changes the tox.ini. It would be nice if you could take of this.

@asottile Thanks for taking care of this!

Holzhaus · 2022-10-05T14:49:16Z

Looks like CI passed except for the failing test_pre_merge_commit_integration on Windows, but that one also fails on current main so it's not related to this PR. Anything else to do?

asottile

Holzhaus · 2022-10-20T09:23:27Z

Any ETA when this will land in a stable release?

I have a rust pre-commit hook that I want to use on a non-rust project and don't want to bother contributors with setting up rust themselves, so my plan is to pin a rust toolchain and set minimum_pre_commit_version to 2.21.0 to prevent people from running into issues.

Mixxx is a C project, and requiring contributors to set up a rust installation on their systems just to be able to use `qml_formatter` is a bit tedious. Fortunately, pre-commit 2.21.0+ features support for bootstrapping Rust toolchains using rustup, so that no preexisting system install is necessary. See pre-commit/pre-commit#2534 for details. We set pre-commit 2.21.0 as the minimum required version, to prevent users that use an older pre-commit version and don't have rust installed from running into problems and to inform them that they should update. If someone uses an pre-commit version < 2.21.0, the following error message will be shown: $ pre-commit run An error has occurred: InvalidConfigError: ==> File .pre-commit-config.yaml ==> At Config() ==> At key: minimum_pre_commit_version =====> pre-commit version 2.21.0 is required but version 2.20.0 is installed. Perhaps run `pip install --upgrade pre-commit`. Check the log at /home/user/.cache/pre-commit/pre-commit.log

asottile · 2022-10-20T13:34:54Z

it's sorta rude to demand releases like that -- it'll happen when it happens

Holzhaus · 2022-10-20T15:58:21Z

it's sorta rude to demand releases like that -- it'll happen when it happens

I'm sorry, that was not my intention. I didn't mean to be pushy. I was just interested because I don't know what the release roadmap for pre-commit is. If there is something blocking the release, I can try to help out.

Mixxx is a C project, and requiring contributors to set up a rust installation on their systems just to be able to use `qml_formatter` is a bit tedious. Fortunately, pre-commit 2.21.0+ features support for bootstrapping Rust toolchains using rustup, so that no preexisting system install is necessary. See pre-commit/pre-commit#2534 for details. We set pre-commit 2.21.0 as the minimum required version, to prevent users that use an older pre-commit version and don't have rust installed from running into problems and to inform them that they should update. If someone uses an pre-commit version < 2.21.0, the following error message will be shown: $ pre-commit run An error has occurred: InvalidConfigError: ==> File .pre-commit-config.yaml ==> At Config() ==> At key: minimum_pre_commit_version =====> pre-commit version 2.21.0 is required but version 2.20.0 is installed. Perhaps run `pip install --upgrade pre-commit`. Check the log at /home/user/.cache/pre-commit/pre-commit.log

Holzhaus mentioned this pull request Sep 30, 2022

[RFC] Migrate from qmlformat to qml_formatter mixxxdj/mixxx#10929

Merged

3 tasks

Holzhaus force-pushed the rust-as-1st-class-language branch 3 times, most recently from 06b5339 to 83825f1 Compare October 2, 2022 11:50

asottile reviewed Oct 3, 2022

View reviewed changes

Holzhaus force-pushed the rust-as-1st-class-language branch 2 times, most recently from 22b2f38 to 85e147c Compare October 4, 2022 12:36

Holzhaus mentioned this pull request Oct 4, 2022

Add ability to specify rust language version #1863

Closed

1 task

asottile reviewed Oct 4, 2022

View reviewed changes

Holzhaus force-pushed the rust-as-1st-class-language branch 4 times, most recently from 9e48d41 to 852d532 Compare October 5, 2022 12:35

asottile reviewed Oct 5, 2022

View reviewed changes

Holzhaus force-pushed the rust-as-1st-class-language branch from 852d532 to cef3842 Compare October 5, 2022 13:23

Holzhaus requested a review from asottile October 7, 2022 09:54

Rust as 1st class language

eb469c7

asottile force-pushed the rust-as-1st-class-language branch from f7c0152 to eb469c7 Compare October 11, 2022 00:05

asottile approved these changes Oct 11, 2022

View reviewed changes

asottile merged commit 529b1a6 into pre-commit:main Oct 11, 2022

Holzhaus deleted the rust-as-1st-class-language branch October 11, 2022 06:30

This was referenced Feb 1, 2023

Better document Rust's first-class support pre-commit/pre-commit.com#783

Merged

Improve pre-commit framework docs sirwart/ripsecrets#58

Merged

Uh oh!

Conversation

Holzhaus commented Sep 30, 2022

Uh oh!

Holzhaus commented Oct 2, 2022

Uh oh!

asottile commented Oct 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asottile commented Oct 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Holzhaus commented Oct 4, 2022

Uh oh!

asottile commented Oct 4, 2022

Uh oh!

Holzhaus commented Oct 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Holzhaus commented Oct 5, 2022

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

Holzhaus commented Oct 20, 2022

Uh oh!

asottile commented Oct 20, 2022

Uh oh!

Holzhaus commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Holzhaus commented Oct 20, 2022 •

edited

Loading