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

rustpkg: Move code from lib.rs to run_cmd.rs and perform.rs #11379

Closed
wants to merge 8 commits into from
Closed

rustpkg: Move code from lib.rs to run_cmd.rs and perform.rs #11379

wants to merge 8 commits into from

Conversation

nielsle
Copy link
Contributor

@nielsle nielsle commented Jan 7, 2014

Move some code from rustpkg::lib.rs to rustpkg::run_cmd.rs and rustpkg::perform.rs

  • run_cmd::run_cmd(command, args, context) acts on on parsed command line parameters to build, install, info.. etc.
  • perform::build() and perform::install() does the actual work of installing and building
  • build_context is initialized a little later in the build process. (Don't initialize it if we just want to run info)
  • The trait CtxMethods is removed
  • PkgScript is moved to package_scripts.rs

@alexcrichton
Copy link
Member

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?

@nielsle
Copy link
Contributor Author

nielsle commented Jan 11, 2014

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.

@alexcrichton
Copy link
Member

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!)

@kud1ing
Copy link

kud1ing commented Jan 11, 2014

Also note, that functionality got lost in a previous move: #11466

@nielsle nielsle closed this Jan 11, 2014
@nielsle nielsle deleted the rustpkg_run_cmd branch January 11, 2014 21:54
flip1995 pushed a commit to flip1995/rust that referenced this pull request Aug 24, 2023
…=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.
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.

3 participants