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

Add list-bin support for scripts. #7925

Merged
merged 2 commits into from
Jan 28, 2022
Merged

Conversation

bacchanalia
Copy link
Collaborator

Small PR to add script support for list-bin. It includes a test.
Because of #7774, I do not believe there is any documentation that needs to be updated.

Related: #7073

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splendid.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect companion of your main pr improving cabal script, including a regression test, thanks!

@andreasabel
Copy link
Member

andreasabel commented Jan 13, 2023

@bacchanalia wrote:

Small PR to add script support for list-bin. It includes a test.

Issue 1: I checked the test output but it does not contain the path to the binary. I'd expect that list-bin produces this path. It is actually doing that for the released cabal-install-3.8, but not on master, likely due to

Anyhow, the test should really include the produced executable path, and I think it didn't due to #8400.
That the path is not included in the test output seemed to have slipped your attention and also passed unnoticed through the review (ping @Mikolaj @jneira).

Because of #7774, I do not believe there is any documentation that needs to be updated.

Issue 2: There is documentation of list-bin now, but does not include your use case with scripts. This is

Would you be able to supply this documentation now to close #7986?

@Mikolaj
Copy link
Member

Mikolaj commented Jan 13, 2023

Indeed I missed the test output problem and the above suggestions make sense to me.


env <- getTestEnv
cacheDir <- getScriptCacheDirectory $ testCurrentDir env </> "script.hs"
assertOutputContains cacheDir res
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bacchanalia : I don't understand how this assertOutputContains succeeds with above output (that does not contain something resembling a cacheDir). But apparently it does. How?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants