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

Clippy Python scripts portablility #2882

Closed
mati865 opened this issue Jun 29, 2018 · 16 comments
Closed

Clippy Python scripts portablility #2882

mati865 opened this issue Jun 29, 2018 · 16 comments
Labels
E-help-wanted Call for participation: Help is requested to fix this issue.

Comments

@mati865
Copy link
Contributor

mati865 commented Jun 29, 2018

Currently all Python scripts in this repository use shebang #!/usr/bin/env python.

The problem

This is bad because python means default system Python which is Python 3 for Arch and scripts don't work.
Other distributions are going to follow it sooner or later because Python 2 won't be maintained since 2020, for an example: Fedora proposal. Also many distributions like Ubuntu, OpenSUSE, Fedora want to ship only with Python 3 preinstalled.

Behaviour on different platforms

I've checked many Linux distributions and all of them had python2, python2.7, python3, python3.X (X differs based on the version).
All I know about OSX is it doesn't have python2 but has python2.7, it would be great if somebody could reply with list of all variants.
MSYS2 (aka windows-gnu) does it just like Linux, no idea about windows-msvc (does #!/usr/bin/env even work there?).

Possible solutions

  1. Use #!/usr/bin/env python2; probably would work everywhere (sometimes it's not installed but available in repo) but note that, Python 2 is almost dead already.
  2. Port scrips to Python 3; I have no idea about OSX and MSVC, should work everywhere else.
  3. RIIR; everything will work on every supported platform, it's win-win but takes more effort.

cc no idea

@oli-obk oli-obk added the E-help-wanted Call for participation: Help is requested to fix this issue. label Jun 29, 2018
@llogiq
Copy link
Contributor

llogiq commented Jun 29, 2018

Alternative: Port the scripts to Rust.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 29, 2018

RIIR all the things

@mati865
Copy link
Contributor Author

mati865 commented Jun 29, 2018

I feel so dump I haven't thought about it earlier.
Edited OP.

@ghost
Copy link

ghost commented Jul 8, 2018

How would option 3 work? Would you create a utility binary crate or somehow use a Rust build script?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 8, 2018

Yea, just add a crate and maybe a bat and an sh script for convenient invocation together with a .rustup file pointing to stable so the utils aren't rebuilt whenever nightly is bumped

@phansch
Copy link
Member

phansch commented Jul 11, 2018

I guess portability is a good reason to use Rust here. Another good thing about using Rust would be that we can add some unit tests to these scripts. The scripts are somewhat complicated already. I remember that the update_lints script had issues before and there are no tests that prevent those issues from appearing again.

@phansch
Copy link
Member

phansch commented Jul 17, 2018

I would be happy to work on this, actually.

@oli-obk Did you mean the rust-toolchain file instead of .rustup?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2018

yes

@llogiq
Copy link
Contributor

llogiq commented Jul 18, 2018

I have looked a bit into this during my ongoing vacation and I believe this could even be written without using regex so a simple .rs file that implements all the functions would work good enough.

@phansch
Copy link
Member

phansch commented Jul 22, 2018

@llogiq Were you thinking of using Clippys lint registry to do it without Regex? I have a basic version that uses regex now, and it seems simple enough to continue.

@llogiq
Copy link
Contributor

llogiq commented Jul 22, 2018

No, just str::split and friends. But using regex is fine, too.

@phansch
Copy link
Member

phansch commented Oct 17, 2018

Quick summary of what's left to do for update_lints.py:

  • Handle -c/--check flag, and use it in CI
  • Generate the pub mod xxx section (gen_mods in update_lints.py)
  • Generate the module::lint_name section (gen_group in update_lints.py)
  • Generate the deprecated lints section (gen_deprecated in update_lints.py)

There's also export.py and lintlib.py which need to be rewritten, too.

bors bot added a commit that referenced this issue Nov 1, 2018
3388: RIIR update lints: Generate deprecated lints r=flip1995 a=phansch

The update script now also generates the 'register_removed' section in
`clippy_lints/src/lib.rs`.

Also, instead of using `let mut store ...`, I added a new identifier
line so that the replacement will continue to work in case `let mut
store ...` ever changes.

cc #2882

Co-authored-by: Philipp Hansch <dev@phansch.net>
bors bot added a commit that referenced this issue Nov 2, 2018
3161: New lint: Unknown clippy lints r=phansch a=flip1995

This is the Clippy version of the `rustc` lint `unknown_lints`. The behavior of this lint is pretty much the same.

Before this is merged a small change in the compiler needs to be done: `CheckLintNameResult` needs to be public. See rust-lang/rust#54106

3387: Replace big if/else expression with match r=flip1995 a=mikerite



3388: RIIR update lints: Generate deprecated lints r=phansch a=phansch

The update script now also generates the 'register_removed' section in
`clippy_lints/src/lib.rs`.

Also, instead of using `let mut store ...`, I added a new identifier
line so that the replacement will continue to work in case `let mut
store ...` ever changes.

cc #2882

Co-authored-by: flip1995 <9744647+flip1995@users.noreply.github.com>
Co-authored-by: flip1995 <hello@philkrones.com>
Co-authored-by: Michael Wright <mikerite@lavabit.com>
Co-authored-by: Philipp Hansch <dev@phansch.net>
bors bot added a commit that referenced this issue Nov 2, 2018
3388: RIIR update lints: Generate deprecated lints r=phansch a=phansch

The update script now also generates the 'register_removed' section in
`clippy_lints/src/lib.rs`.

Also, instead of using `let mut store ...`, I added a new identifier
line so that the replacement will continue to work in case `let mut
store ...` ever changes.

cc #2882

Co-authored-by: Philipp Hansch <dev@phansch.net>
bors bot added a commit that referenced this issue Nov 3, 2018
3399: RIIR update lints: Generate modules section and lint group sections r=flip1995 a=phansch

This adds the last missing parts of the generating code.

cc #2882

Co-authored-by: Philipp Hansch <dev@phansch.net>
bors bot added a commit that referenced this issue Nov 5, 2018
3408: RIIR update lints: Add check mode (update_lints.py rewrite complete) r=oli-obk a=phansch

This finishes up the rewrite of `update_lints.py` in Rust. More
specifically, this

* adds the `--check` flag and handling to clippy_dev
* tracks file changes over the different calls to `replace_region_in_file`
* only writes changes to files if the `--check` flag is *not* used
* runs `./util/dev update_lints --check` on CI instead of the old script
* replaces usage of the `update_lints.py` script with an error

`./util/dev update_lints` behaves 99% the same as the python script.
The only difference that I'm aware of is an ordering change to
`clippy_lints/src/lib.rs` because underscores seem to be sorted
differently in Rust and in Python.

:checkered_flag:

cc #2882 

Co-authored-by: Philipp Hansch <dev@phansch.net>
Co-authored-by: Philipp Krones <hello@philkrones.com>
@mati865
Copy link
Contributor Author

mati865 commented Oct 16, 2020

I think this is done.

@mati865 mati865 closed this as completed Oct 16, 2020
@ThibsG
Copy link
Contributor

ThibsG commented Oct 16, 2020

Actually I am not sure it is totally done, unless these two rewriting have been dropped or done by other means?

There's also export.py and lintlib.py which need to be rewritten, too.

@mati865
Copy link
Contributor Author

mati865 commented Oct 16, 2020

They seem to be used only on the CI so there is no portability issue for the developers.

@llogiq
Copy link
Contributor

llogiq commented Oct 16, 2020

True, we don't have a clippy_dev subcommand to create the lints.json yet, right? lintlib shouldn't be needed anymore AFAIR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Development

No branches or pull requests

5 participants