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

replace libterm with termcolor in libtest #63725

Closed
wants to merge 1 commit into from

Conversation

euclio
Copy link
Contributor

@euclio euclio commented Aug 20, 2019

This commit also completely removes libterm from the tree.

Fixes #60349.
Fixes #45728.

This commit also completely removes libterm from the tree.
@rust-highfive
Copy link
Collaborator

r? @rkruppe

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2019
@ehuss
Copy link
Contributor

ehuss commented Aug 20, 2019

I'm concerned about bumping the termcolor version. There is a bug between termcolor and the fwdansi crate that causes colors to go horribly wrong in cargo on windows 7 and 8. I think it would be good to get kennytm/fwdansi#1 fixed before bumping termcolor.

Colors are currently broken, but they just don't display. If termcolor is updated, then the colors don't reset and it looks awful. See #55769

@alexcrichton
Copy link
Member

In addition to @ehuss's concern the build process for libtest is changing in #63637 which will conflict with this PR unfortunately. There's a few issues with that which are:

  • The manifest of termcolor and its dependencies will need the rustc-dep-of-std treatment (before they didn't, but after that PR they will)
  • We've never gotten around to getting winapi building as part of libstd or for libtest, so termcolor's transitive dependency will be difficult to manage. We've basically just never tried this, but it's likely to be thorny for at least one reason or another.

@crlf0710
Copy link
Member

crlf0710 commented Aug 21, 2019

I coincidentally have a fork of termcolor that had got rid of the winapi dependency at https://github.com/crlf0710/termcolor , if anyone found it useful, feel free to cherry pick that commit away. I had named the feature "rustc-dep-of-test" but people might want to change it.

Also, i'm very grateful to #63637, i tried to do this change before in #62829 but it was so difficult to get it right.
@alexcrichton @euclio

@euclio
Copy link
Contributor Author

euclio commented Aug 21, 2019

Nice, we can certainly try that. Though I'm curious what issues people foresee with trying to get winapi to build in-tree. It might be nice in the long term for libstd to start depending on it anyways.

@bors
Copy link
Contributor

bors commented Aug 24, 2019

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

@crlf0710
Copy link
Member

@euclio need a rebase

@retep998
Copy link
Member

I would be happy to help with getting winapi building as part of libstd/libtest if that would make things easier.

@alexcrichton
Copy link
Member

As mentioned by @crlf0710 now that #63637 has landed this will require both a rebase and a more substantial update. I would personally still love to see src/libterm deleted and would love to transition over to termcolor, so if any guidance is needed please let me know!

@retep998 I'd personally also love to delete src/libstd/sys/windows/c.rs and rely on upstream winapi definitions instead. I don't recall exact sets of issues but I think the only possible blocker would be the custom import libraries of winapi. I suspect though you've thought about the issue before, and if we can solve that then I think depending on winapi can happen at any time with just a PR!

@retep998
Copy link
Member

Is the issue with linking to the custom import libraries, or depending on the import library crates at all? For the former rustbuild already sets an environment variable to not do so, but for the latter there would need to be a way to opt out perhaps via a default cargo feature.

@alexcrichton
Copy link
Member

I'm just hypothesizing I don't actually know if there are any issues. The best way to find out is to actually make the change and see what happens or what goes wrong.

@euclio
Copy link
Contributor Author

euclio commented Sep 7, 2019

I started by trying to add a rustc-dep-of-std feature to winapi, but cargo complains because winapi already has a feature named std. Is there a way to work around this?

@crlf0710
Copy link
Member

crlf0710 commented Sep 7, 2019

@alexcrichton Sorry, i just checked and it seems rustc-std-workspace-std on crates.io is totally blank (I think i made a mistake when uploading). Could you add the necessary pub use std::*; and publish a new dot version?
After that @euclio can use the optional dependency to replace the feature (just remove the std = [] line, i think.

@alexcrichton
Copy link
Member

Sure, I've published a version with that.

@Alexendoo Alexendoo added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2019
@Alexendoo
Copy link
Member

Ping from triage: How is adapting to #63637 going @euclio?

@euclio
Copy link
Contributor Author

euclio commented Sep 19, 2019

This is blocked on updating termcolor and its dependencies with rustc-std-workspace support.

@Alexendoo Alexendoo added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 19, 2019
@crlf0710
Copy link
Member

crlf0710 commented Sep 19, 2019

Previously I experimented with the changes locally and found that now bootstrap relies on the in-tree libcore, and of course that will fail... My local installed cargo-tree doesn't support the newest Cargo.lock format (and i can't get it to work with cargo 0.40), so i can't really tell what's the problem.

I don't really know how to get it right... so tricky...

@crlf0710
Copy link
Member

Thinking aloud: Why does libtest needs a term library anyway... cargo test command can simply support colors with the fwdansi style handling, right? @alexcrichton

@bjorn3
Copy link
Member

bjorn3 commented Sep 20, 2019

@crlf0710 It is possible to build and invoke a test binary directly without using cargo test.

@crlf0710
Copy link
Member

@bjorn3 It is, but ... what for?

@bjorn3
Copy link
Member

bjorn3 commented Sep 20, 2019

For example when you want to cross compile and then run the test executable on the target machine.

@Mark-Simulacrum
Copy link
Member

I'm going to close this PR as pending (likely significant) work in the ecosystem to update the dependencies to be ready for std-inclusion via rustc-dep-of-std propagation.

We can reopen once it's ready to move forward.

@Mark-Simulacrum Mark-Simulacrum added S-blocked-closed and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Oct 24, 2019
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 29, 2020
@jyn514 jyn514 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-blocked-closed labels Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet