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

Don't compile ansi_term on Windows #1155

Closed
Tracked by #1037
XAMPPRocky opened this issue Jan 27, 2018 · 14 comments · Fixed by #1178
Closed
Tracked by #1037

Don't compile ansi_term on Windows #1155

XAMPPRocky opened this issue Jan 27, 2018 · 14 comments · Fixed by #1178
Labels
C-enhancement Category: Raise on the bar on expectations

Comments

@XAMPPRocky
Copy link

ansi_term currently doesn't work on 32bit Windows platforms and is not being maintained. clap being dependent by so many crates(mine included) needs to resolve this since there is no maintainer for ansi_term. There are two solutions I can think of.

  • clap maintains a forked version of ansi_term.
  • clap pulls required functionality of our ansi_term and into it's own module in tree.
@SergioBenitez
Copy link
Contributor

Alternatively, migrate to yansi, a crate I created for Rocket. The API is fairly similar to ansi_term's, so migrating should be very simple. If there's a feature missing in yansi that makes migrating difficult, I'm happy to adjust the API.

@BurntSushi
Copy link
Contributor

@SergioBenitez Your crate only supports ANSI escape sequences, which won't work in Windows consoles that don't have ANSI support. (Your API is also appears fundamentally incompatible with how Windows console coloring works.)

@SergioBenitez
Copy link
Contributor

@BurntSushi Yes, I'm quite aware of that; it's even part of the crate's name! This issue is about moving away from ansi_term, which also doesn't support older Windows consoles.

@BurntSushi
Copy link
Contributor

@SergioBenitez Let me rephrase: It would be great to move to a library that supports Windows console coloring.

@kbknapp
Copy link
Member

kbknapp commented Jan 31, 2018

@Aaronepower ansi_term isn't used by clap on Windows, as I just default to no color support on Windows. However, the plan is to move to termcolor to enable color support for Windows. Progress can be tracked specifically in #836

However, this color re-write will be implemented in v3 (which actively being worked again after a stagnant period) and can be tracked via the overall v3 tracking issue #1037

I've implemented a feature freeze for v2 so I can actually get v3 out the door. So it shouldn't be long before v3-alpha1 is out.

I'm going to close this so comments can be consolidated in one place. I appreciate the suggestions @SergioBenitez and if termcolor ends up not working out and I have to default back to no Windows color support, but still want color support elsewhere I'll look into swapping ansi_term with yansi.

@kbknapp kbknapp closed this as completed Jan 31, 2018
@XAMPPRocky
Copy link
Author

@kbknapp It's still compiled on Windows which is the real problem.

@kbknapp
Copy link
Member

kbknapp commented Jan 31, 2018

@Aaronepower Ah, I see my mistake. I'll re-open this issue as "Don't compile ansi_term on Windows which should be totally possible with cargo.

@kbknapp kbknapp reopened this Jan 31, 2018
@kbknapp kbknapp changed the title Move away from using ansi_term. Don't compile ansi_term on Windows Jan 31, 2018
@kbknapp kbknapp added D: easy E-medium Call for participation: Experience needed to fix: Medium / intermediate E-easy Call for participation: Experience needed to fix: Easy / not much labels Jan 31, 2018
@kbknapp
Copy link
Member

kbknapp commented Jan 31, 2018

I'd be willing to merge a PR for 2.29.3 that fixes this.

@kbknapp kbknapp added this to the 2.29.3 milestone Feb 2, 2018
@kbknapp
Copy link
Member

kbknapp commented Feb 4, 2018

Turns out this is harder than I had first thought: rust-lang/cargo#1197

@kbknapp kbknapp removed this from the 2.29.3 milestone Feb 4, 2018
@kbknapp kbknapp added W: postponed and removed D: easy E-medium Call for participation: Experience needed to fix: Medium / intermediate E-easy Call for participation: Experience needed to fix: Easy / not much labels Feb 4, 2018
@kbknapp
Copy link
Member

kbknapp commented Feb 12, 2018

For those compiling only to windows, not using the color feature will disable compiling ansi_term

kevin@beefcake: ~/Projects/clap-rs 
➜ cargo build --no-default-features --features wrap_help,suggestions
   Compiling strsim v0.7.0
   Compiling libc v0.2.36
   Compiling bitflags v1.0.1
   Compiling unicode-width v0.1.4
   Compiling term_size v0.3.1
   Compiling textwrap v0.9.0
   Compiling clap v2.29.4 (file:///home/kevin/Projects/clap-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 8.0 secs

I'm going to close this for now, and re-address once cargo supports per-target deps. I'll also keep this in mind during the great color redux in v3.

@kbknapp kbknapp closed this as completed Feb 12, 2018
@retep998
Copy link

re-address once cargo supports per-target deps

You mean like [target.i686-pc-windows-gnu.dependencies] or [target.'cfg(all(windows, target_arch="x86"))'.dependencies]? Or did you mean per-target features?

@kbknapp
Copy link
Member

kbknapp commented Feb 12, 2018

@retep998 I meant them somewhat interchangeably. The issue being the color feature is currently ignored on Windows, yet even though it's ignored the ansi_term dep is still compiled.

What I'd like to happen is per-target features (so that on everything but Windows the color feature will include ansi_term, but on Windows the color feature will simply be empty, thus not compiling ansi_term for no reason).

If this is already possible, I may have misunderstood the state of things.

@retep998
Copy link

While features themselves cannot be target specific, dependencies can, even if they are optional. You can enable the feature from any target, but the dependency will only be compiled for the targets it specifies.

@kbknapp
Copy link
Member

kbknapp commented Feb 12, 2018

TIL...just tested it and it works!

Wow 🎉 Thanks for the explanation @retep998 👍

I'll re-open this and put in the PR.

@kbknapp kbknapp reopened this Feb 12, 2018
kbknapp added a commit that referenced this issue Feb 12, 2018
Before this commit, ansi_term was compiled anytime the `color` feature
was used. However, on Windows the `color` feature is ignored. Even so
ansi_term was compiled, and just not used. This commit fixes that by
only compiling ansi_term on non-Windows targets. Thanks to @retep998 for
the gudiance.

Closes #1155
@kbknapp kbknapp added C-enhancement Category: Raise on the bar on expectations E: optional dep and removed W: 3.x labels Feb 12, 2018
@kbknapp kbknapp mentioned this issue Feb 13, 2018
87 tasks
kbknapp added a commit that referenced this issue Feb 13, 2018
Before this commit, ansi_term was compiled anytime the `color` feature
was used. However, on Windows the `color` feature is ignored. Even so
ansi_term was compiled, and just not used. This commit fixes that by
only compiling ansi_term on non-Windows targets. Thanks to @retep998 for
the gudiance.

Closes #1155
@kbknapp kbknapp mentioned this issue Feb 13, 2018
volks73 added a commit to volks73/cargo-wix that referenced this issue Apr 15, 2018
The [clap](https://crates.io/crates/clap) crate used for argument
parsing uses the `ansi_term` crate for colorized output, but colorized
output is disabled for Windows. However, `ansi_term` is still compiled
and used on Windows unless the build is configured to disable the
"color" feature as discussed here:
clap-rs/clap#1155 (comment)

The manifest is updated to disable the color feature following these
instructions:
https://github.com/kbknapp/clap-rs#optional-dependencies--features
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants