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
Aaronepower opened this Issue Jan 27, 2018 · 14 comments

Comments

Projects
None yet
5 participants
@Aaronepower

Aaronepower commented Jan 27, 2018

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

This comment has been minimized.

SergioBenitez commented Jan 30, 2018

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

This comment has been minimized.

Contributor

BurntSushi commented Jan 30, 2018

@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

This comment has been minimized.

SergioBenitez commented Jan 30, 2018

@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

This comment has been minimized.

Contributor

BurntSushi commented Jan 30, 2018

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

@kbknapp

This comment has been minimized.

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 Jan 31, 2018

@Aaronepower

This comment has been minimized.

Aaronepower commented Jan 31, 2018

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

@kbknapp

This comment has been minimized.

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 from Move away from using ansi_term. to Don't compile ansi_term on Windows Jan 31, 2018

@kbknapp

This comment has been minimized.

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

This comment has been minimized.

Member

kbknapp commented Feb 4, 2018

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

@kbknapp

This comment has been minimized.

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 Feb 12, 2018

@retep998

This comment has been minimized.

retep998 commented Feb 12, 2018

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

This comment has been minimized.

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

This comment has been minimized.

retep998 commented Feb 12, 2018

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

This comment has been minimized.

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

imp(Deps): doesnt compile ansi_term on Windows since its not used
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 referenced this issue Feb 13, 2018

Open

Tracking Issue for 3.x #1037

51 of 90 tasks complete

kbknapp added a commit that referenced this issue Feb 13, 2018

imp(Deps): doesnt compile ansi_term on Windows since its not used
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 referenced this issue Feb 13, 2018

Merged

V3 dev #1182

volks73 added a commit to volks73/cargo-wix that referenced this issue Apr 15, 2018

Remove ansi_term dependency for clap
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