-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
rustpkg: Move code from lib.rs to run_cmd.rs and perform.rs #11379
Conversation
I'm a little unclear on what's going on here. Some of these changes seem fairly contentious in that they go backwards a bit in style. This also looks like it has a lot of changes inflated amongst others. This also appears to only remove tests rather than add them. It sounds like functionality is being added, but no tests are being added as well? |
As I see it rustpkg should not initialize the BuildContext before it is needed. The BuildContext mainly consists of WorkCache, and that is not needed for commands like "rustpkg init". Therefore my main goal was to initialize BuildContext later in the build process. Right now "rustpkg init" calls a method on the trait CtxMethods which is implemented by BuildContext, so you cannot run "rustpkg init" without initializing BuildContext. Therefore I would like to replace the methods of CtxMethods with functions. If you want to move run_cmd to a separate file, then you also need to move package_script to avoid circular dependencies, but that should probably have been a later commit. I will retract this pull request and make a smaller request that removes CtxMethods. |
I certainly don't want to discourage any work on rustpkg, this looks like it's certainly improvement! I personally don't have huge experience in rustpkg, so I was just looking for a little more explanation on what's going on and why it's going on, although I agree that smaller pull requests would be nicer (always easier to review!) |
Also note, that functionality got lost in a previous move: #11466 |
…=xFrednet Fix tuple_array_conversions lint on nightly ``` changelog: ICE: [`tuple_array_conversions`]: Don't expect array length to always be usize ``` tl;dr: changed [`Const::eval_target_usize`](https://github.com/rust-lang/rust/blob/master/compiler/rustc_middle/src/ty/consts.rs#L359) to [`Consts::try_eval_target_usize`](https://github.com/rust-lang/rust/blob/master/compiler/rustc_middle/src/ty/consts.rs#L327) to get rid of ICE. I have encountered a problem with clippy: it caught ICE when working with a codebase that uses a lot of nightly features. Here's a (stripped) ICE info: ``` error: internal compiler error: /rustc/5c6a7e71cd66705c31c9af94077901a220f0870c/compiler/rustc_middle/src/ty/consts.rs:361:32: expected usize, got Const { ty: usize, kind: N/rust-lang#1 } thread 'rustc' panicked at /rustc/5c6a7e71cd66705c31c9af94077901a220f0870c/compiler/rustc_errors/src/lib.rs:1635:9: Box<dyn Any> stack backtrace: ... 16: 0x110b9c590 - rustc_middle[449edf845976488d]::util::bug::bug_fmt 17: 0x102f76ae0 - clippy_lints[71754038dd04c2d2]::tuple_array_conversions::all_bindings_are_for_conv ... ``` I don't really know what's going on low-level-wise, but seems like this lin assumed that the length of the array can always be treated as `usize`, and *I assume* this doesn't play well with `feat(generic_const_exprs)`. I wasn't able to build a minimal reproducible example, but locally this fix does resolve the issue.
Move some code from rustpkg::lib.rs to rustpkg::run_cmd.rs and rustpkg::perform.rs