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

Separate System.Console.ANSI.Types to its own package #151

Closed
Bodigrim opened this issue Feb 18, 2023 · 13 comments
Closed

Separate System.Console.ANSI.Types to its own package #151

Bodigrim opened this issue Feb 18, 2023 · 13 comments

Comments

@Bodigrim
Copy link
Contributor

tl;dr I would like to make ansi-terminal an optional dependency of tasty, but cannot do so unless System.Console.ANSI.Types is separated to its own package.

Current relationship between ansi-terminal and tasty is kinda straining for both packages. On one side, as discussed in #113 (comment), ansi-terminal is blocked from acquiring new dependencies on even something as basic as text because it is likely to cause circular dependencies. On the other side, even current lightweight footprint is not enough for some boot packages such as filepath, which ended up implementing a micro-testing framework of its own.

I would like to make ansi-terminal an optional dependency of tasty. On the surface it is quite easy: if there is a build plan with ansi-terminal, use it, but if there is none, then sacrifice colors and carry on. In such case both ansi-terminal would be able to evolve at its own pace, and tasty would be suitable for filepath test suite (which is currently prohibited by a circular dependency filepath -> tasty -> ansi-terminal -> Win32 -> filepath.

The only blocker at the moment is that Test.Tasty.Providers.ConsoleFormat re-exports some types from System.Console.ANSI.Types. If only the latter was in its own package, used both by tasty and ansi-terminal, the puzzle would be solved.

How does it sound? I'm happy to do all the work associated.

See also UnkindPartition/tasty#361.

@Bodigrim
Copy link
Contributor Author

And also haskell/filepath#181.

@mpilgrem
Copy link
Collaborator

I don't see any reason not to do this. If I understand correctly, it helps some users and does not affect others.

I assume both packages would remain in this repository, each in its own subfolder named after the package. I assume that the new package would be called ansi-terminal-types (looking at how other packages have been named on Hackage).

@Bodigrim
Copy link
Contributor Author

Awesome. Shall I give it a try?

@mpilgrem
Copy link
Collaborator

@Bodigrim, please do.

@mpilgrem
Copy link
Collaborator

mpilgrem commented Mar 17, 2023

@Bodigrim, I have some time and thought I might take steps to implement this, step-by-step. Let me know if that cuts across what you may be doing behind the scenes. I propose the following sequential steps:

  1. Move the existing ansi-terminal package to a folder ansi-terminal in the repository. Check that the CI and other repository 'infrastructure' still works.
  2. Create an ansi-terminal-types package in folder ansi-terminal-types in the repository. Modify the CI, and check it passes. Update any repository infrastructure, accordingly. When all is 'green', release to Hackage. Add to Stackage.
  3. Change the the ansi-terminal package to depend on ansi-terminal-types. Update any repository infrastructure, accordingly. When all is 'green' and ansi-terminal-types is in Stackage snapshots, release that version of ansi-terminal to Hackage.

@mpilgrem
Copy link
Collaborator

I have implemented Step 1.

@mpilgrem
Copy link
Collaborator

mpilgrem commented Mar 17, 2023

mpilgrem added a commit that referenced this issue Mar 18, 2023
Also drops support for GHC versions before GHC 7.10.1, because those versions of GHC did not support re-exported modules.
mpilgrem added a commit that referenced this issue Mar 18, 2023
Also drops support for GHC versions before GHC 7.10.1, because those versions of GHC did not support re-exported modules.
@mpilgrem
Copy link
Collaborator

I have also implemented Step 3, but not yet released ansi-terminal-0.11.5 on Hackage.

@Bodigrim
Copy link
Contributor Author

@mpilgrem sorry for delay, I'm AFK. I actually took a stab at it back in February: https://github.com/Bodigrim/tasty/tree/optional-ansi-terminal and https://github.com/Bodigrim/ansi-terminal/tree/split-types. Howeve, I did not submit the PR, because unfortunately it does not solve the motivating case. The problem is that tasty depends on ansi-terminal not only directly, but also via optparse-applicative / ansi-wl-pprint. Sorry for not reporting this back earlier.

That said, I think in the grand scheme of things this split is a healthy development, even if not bearing immediate fruit. Thanks for making it happen.

@mpilgrem
Copy link
Collaborator

Thinking about your underlying problem, I wondered why/how the Win32 package depends on the filepath package. Before Win32-2.5.0.0, it did not. Win32-2.5.0.0 saw new System.Win32.Mintty.cygwinMSYSCheck (not exported) rely on System.FilePath.takeFileName. The Cabal file for Win32-2.13.2.1 says the package depends on filepath, but I can't see that it, in fact, imports any of the System.FilePath ... modules; System.Win32.Mintty no longer imported System.FilePath. Win32-2.13.3.0 (July 2022) added AFPP support and the System.Win32.WindowsString.x modules - they introduce a dependency on filepath.

@Bodigrim
Copy link
Contributor Author

Another work around is that after #156 ansi-terminal can potentially vendor in the only function from System.Win32.Mintty it uses and drop Win32 from dependencies.

@mpilgrem
Copy link
Collaborator

@Bodigrim, I have been thinking about that (bringing all residual use of Microsoft's Win32 API back 'in house', once emulation falls away). However, it is not just isMinTTYHandle - the Win32 API is needed to read characters emitted into the input stream - for the getReported functionality. Most of it is already 'in house', however, either because it started life in ansi-terminal and then moved out to the Win32 package or because the Win32 package needed patching when the Windows I/O Manager was first introduced.

@Bodigrim
Copy link
Contributor Author

Thanks, @mpilgrem, brilliant!

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

No branches or pull requests

2 participants