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

Pass the current shell width to rustc #7315

Open
wants to merge 2 commits into
base: master
from

Conversation

@estebank
Copy link

estebank commented Aug 31, 2019

Use the feature introduced in rust-lang/rust#63402

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Aug 31, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@estebank estebank force-pushed the estebank:long-lines-trimming branch from 3cd6301 to faac528 Aug 31, 2019
@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Aug 31, 2019

Thanks! I believe -Z flags only work on nightly, right? So this will likely need a corresponding -Z flag in cargo. Those flags are defined in the CliUnstable struct. That module contains all the documentation for how to add it. Additionally -Z help is in cli.rs and finally the docs are in unstable.md.

Also, assuming this flag should be passed to all invocations, I think it would probably go somewhere like add_error_format_and_color. That will get passed to both rustc and rustdoc (it looks like it should be supported in rustdoc?).

This will also need a small test.

This probably won't work well on Windows as-is, because we don't have a good way to detect the window size on mintty/cygwin-style terminals, and it is hard-coded to 60. We may need to special-case the value there with a different hard-coded version, since I would assume 60-column errors will look very poor.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 3, 2019

I'd also actually expect that rustc itself would determine the terminal width. One thing we realized with Cargo is that terminal widths can change over time, and especially because rustc can take awhile it may change from rustc process start to when an error is actually emitted. In that sense it may be best if Cargo/rustc just share the same code for calculating the terminal width?

@estebank

This comment has been minimized.

Copy link
Author

estebank commented Sep 3, 2019

I'd also actually expect that rustc itself would determine the terminal width.

@alexcrichton I would prefer that too, but it seems to me that the way that cargo invokes rustc makes it so that rustc no longer can query the terminal width.

One thing we realized with Cargo is that terminal widths can change over time, and especially because rustc can take awhile it may change from rustc process start to when an error is actually emitted. In that sense it may be best if Cargo/rustc just share the same code for calculating the terminal width?

That makes sense to me, but keep in mind that cargo is more likely to hit bad graphical glitches due to the live progress bar. We're also not redrawing the buffer during reflows in rustc. Handling this sounds to me like a good idea, but I would rather land this as quickly as possible to start getting real world feedback before landing it on stable.

@ehuss makes sense. I'll keep looking. I'd be ok with this not being stabilized in the next cargo, but once it is I would like to have it be opt-out, not opt-in.

This probably won't work well on Windows as-is, because we don't have a good way to detect the window size on mintty/cygwin-style terminals, and it is hard-coded to 60.

Would it be ok to change the function to return Option and do unwrap_or(60) where needed and not pass the flag for this when it's None?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 3, 2019

Oh right yeah, we don't actually give rustc the terminal! In that case this seems fine to experiment with.

@estebank estebank force-pushed the estebank:long-lines-trimming branch from 367dea4 to f92ad42 Sep 3, 2019
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 3, 2019

☔️ The latest upstream changes (presumably #7216) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

ehuss left a comment

Also be sure to run rustfmt (from stable).

@@ -407,6 +419,10 @@ mod imp {
mod imp {
pub(super) use super::default_err_erase_line as err_erase_line;

pub fn accurate_stderr_width() -> Option<usize> {
None

This comment has been minimized.

Copy link
@ehuss

ehuss Sep 4, 2019

Contributor

It seems a bit unfortunate to unconditionally disable on Windows. Perhaps imp::stderr_width() could return an enum with the three states (notty, known, unknown?)? Then maybe it could have a method to convert it to Option<usize> and take care of converting unknown to 60.

tests/testsuite/build.rs Outdated Show resolved Hide resolved
cx.bcx.config.cli_unstable().terminal_width,
cx.bcx.config.shell().accurate_err_width(),
) {
cmd.arg(format!("-Zterminal-width={}", width));

This comment has been minimized.

Copy link
@ehuss

ehuss Sep 4, 2019

Contributor

Is there a reason this isn't included in the pipeline path? Does it not affect the rendered field? We plan on moving all interaction through the json path, so this may not work this way.

This comment has been minimized.

Copy link
@estebank

estebank Sep 22, 2019

Author

I believe the rustc side ignores this codepath for the rendered field in json output, which is unfortunate for the integration here. We'll likely need some back and forth with the other repo to get both to be in the same page about this.

@@ -764,6 +765,14 @@ fn add_error_format_and_color(
} else {
let mut color = true;
match cx.bcx.build_config.message_format {
MessageFormat::Human if nightly_features_allowed() => {

This comment has been minimized.

Copy link
@ehuss

ehuss Sep 4, 2019

Contributor

I noticed this changed it to unconditionally enable it on nightly. We haven't really done that before, but I don't really have an objection to it. We have problems with figuring out how to get people to test unstable flags, so unconditionally enabling it on nightly might be a good idea, since it is just a visual change. I'd like to hear from other cargo members, though.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Sep 23, 2019

Member

Sorry for the late repsponse, but I think I'd prefer if this still needed to be opted-in to if that's ok, because otherwise if -Zterminal-width changes it'd break all nightly users by default until we can ship a fix

This comment has been minimized.

Copy link
@estebank

estebank Sep 27, 2019

Author

Fair enough.

@estebank estebank force-pushed the estebank:long-lines-trimming branch from f92ad42 to 5202881 Sep 22, 2019
@estebank estebank force-pushed the estebank:long-lines-trimming branch from 98aaae0 to d4a3272 Sep 22, 2019
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 15, 2019

☔️ The latest upstream changes (presumably #7450) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Oct 28, 2019

Ping @estebank, is it correct that this is blocked on adding terminal-width support to the JSON rendered field? Let me know if you have any questions or need any help.

@estebank

This comment has been minimized.

Copy link
Author

estebank commented Oct 28, 2019

@ehuss I think that might be the case, I'll try to take some time this weekend to get this fixed on both ends.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 11, 2019
Pkgsrc changes:
 * Remove patch which no longer applies (but what about RPATH?)
 * Adapt a few patches to changed files upstream.

Upstream changes:

Version 1.39.0 (2019-11-07)
===========================

Language
--------
- [You can now create `async` functions and blocks with `async fn`,
  `async move {}`, and `async {}` respectively, and you can now call
  `.await` on async expressions.][63209]
- [You can now use certain attributes on function, closure, and function
  pointer parameters.][64010] These attributes include `cfg`, `cfg_attr`,
  `allow`, `warn`, `deny`, `forbid` as well as inert helper attributes used
  by procedural macro attributes applied to items. e.g.
  ```rust
  fn len(
      #[cfg(windows)] slice: &[u16],
      #[cfg(not(windows))] slice: &[u8],
  ) -> usize {
      slice.len()
  }
  ```
- [You can now take shared references to bind-by-move patterns in the
  `if` guards of `match` arms.][63118] e.g.
  ```rust
  fn main() {
      let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);

      match array {
          nums
  //      ---- `nums` is bound by move.
              if nums.iter().sum::<u8>() == 10
  //                 ^------ `.iter()` implicitly takes a reference to `nums`.
          => {
              drop(nums);
  //          ----------- Legal as `nums` was bound by move and so we have ownership.
          }
          _ => unreachable!(),
      }
  }
  ```

Compiler
--------
- [Added tier 3\* support for the `i686-unknown-uefi` target.][64334]
- [Added tier 3 support for the `sparc64-unknown-openbsd` target.][63595]
- [rustc will now trim code snippets in diagnostics to fit in your terminal.]
  [63402] **Note** Cargo currently doesn't use this feature. Refer to
  [cargo#7315][cargo/7315] to track this feature's progress.
- [You can now pass `--show-output` argument to test binaries to print the
  output of successful tests.][62600]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
---------
- [`Vec::new` and `String::new` are now `const` functions.][64028]
- [`LinkedList::new` is now a `const` function.][63684]
- [`str::len`, `[T]::len` and `str::as_bytes` are now `const` functions.][63770]
- [The `abs`, `wrapping_abs`, and `overflowing_abs` numeric functions are
  now `const`.][63786]

Stabilized APIs
---------------
- [`Pin::into_inner`]
- [`Instant::checked_duration_since`]
- [`Instant::saturating_duration_since`]

Cargo
-----
- [You can now publish git dependencies if supplied with a `version`.]
  [cargo/7237]
- [The `--all` flag has been renamed to `--workspace`.][cargo/7241] Using
  `--all` is now deprecated.

Misc
----
- [You can now pass `-Clinker` to rustdoc to control the linker used
  for compiling doctests.][63834]

Compatibility Notes
-------------------
- [Code that was previously accepted by the old borrow checker, but rejected by
  the NLL borrow checker is now a hard error in Rust 2018.][63565] This was
  previously a warning, and will also become a hard error in the Rust 2015
  edition in the 1.40.0 release.
- [`rustdoc` now requires `rustc` to be installed and in the same directory to
  run tests.][63827] This should improve performance when running a large
  amount of doctests.
- [The `try!` macro will now issue a deprecation warning.][62672] It is
  recommended to use the `?` operator instead.
- [`asinh(-0.0)` now correctly returns `-0.0`.][63698] Previously this
  returned `0.0`.

[62600]: rust-lang/rust#62600
[62672]: rust-lang/rust#62672
[63118]: rust-lang/rust#63118
[63209]: rust-lang/rust#63209
[63402]: rust-lang/rust#63402
[63565]: rust-lang/rust#63565
[63595]: rust-lang/rust#63595
[63684]: rust-lang/rust#63684
[63698]: rust-lang/rust#63698
[63770]: rust-lang/rust#63770
[63786]: rust-lang/rust#63786
[63827]: rust-lang/rust#63827
[63834]: rust-lang/rust#63834
[63927]: rust-lang/rust#63927
[63933]: rust-lang/rust#63933
[63934]: rust-lang/rust#63934
[63938]: rust-lang/rust#63938
[63940]: rust-lang/rust#63940
[63941]: rust-lang/rust#63941
[63945]: rust-lang/rust#63945
[64010]: rust-lang/rust#64010
[64028]: rust-lang/rust#64028
[64334]: rust-lang/rust#64334
[cargo/7237]: rust-lang/cargo#7237
[cargo/7241]: rust-lang/cargo#7241
[cargo/7315]: rust-lang/cargo#7315
[`Pin::into_inner`]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.into_inner
[`Instant::checked_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_duration_since
[`Instant::saturating_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.saturating_duration_since
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.