Skip to content

A dumber approach to reverting #7995 on 3.8 branch #8319

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

Closed
wants to merge 14 commits into from

Conversation

Mikolaj
Copy link
Member

@Mikolaj Mikolaj commented Jul 25, 2022

A dumber (commit by commit) approach to reverting #7995 on 3.8 branch. This is made painful by the fact that we got sloppy and didn't rebase a subsequent PR and so the overall merge got complex instead of fast-forward. Also, some other PRs (at least one, but probably more) actually depend on this PR.

Manual hacks notes:

  • I've left the definition of rawSystemProcAction and fromCreatePipe in the revert of Rework subprocess helpers, because they were causing a conflict and indeed they are used in some subsequent PRs
  • I've re-added logCommand (from the same reverted commits as above) that is used in newer PRs

@jneira
Copy link
Member

jneira commented Jul 25, 2022

Thanks for trying the revert, ci is failing at UNEXPECTED FAIL: PackageTests/NewBuild/CmdRun/Terminate/cabal.test.hs, like mine one: i had to revert #8081 with a796fa9

@Mikolaj
Copy link
Member Author

Mikolaj commented Jul 25, 2022

Let's hope your version works. I remember CI was a flaky with that test, but it's better than failing every time. I'm trying another hack, but at this point it's failing outright. :)

@Mikolaj Mikolaj force-pushed the 3.8 branch 2 times, most recently from bbbd5b1 to 65063a7 Compare July 25, 2022 13:33
@Mikolaj
Copy link
Member Author

Mikolaj commented Jul 25, 2022

This really failed hard with "*** Exception: unexpected: done sleeping", so changing the test harness doesn't seem to be sufficient. The cleanup probably fixed the real code that is used in the test. I will try your idea and revert the change of the test #8081 that adjusts it to the revised code (that we are reverting here).

@Mikolaj
Copy link
Member Author

Mikolaj commented Jul 25, 2022

The same failure with #8081 reverted, so I'm just going to mark the test as known broken. It was flaky before.

@Mikolaj
Copy link
Member Author

Mikolaj commented Jul 25, 2022

If CI passes, I'm going to tag this to get gitlab to build binaries so that @jneira can easily test if this works around #8208.

@robx
Copy link
Collaborator

robx commented Jul 25, 2022

Here's the list of commits that you'd want to revert. It's not a good idea to keep #7921 if reverting #7995, that's very likely to break things.

2521ee9d4 - Fix race condition in PackageTests/NewBuild/CmdRun/Terminate (4 months ago) <Robert Vollmert>
aa4f0ad3c - Add changelog (4 months ago) <Robert Vollmert>
d2172d896 - Add test for terminating "cabal run" on unix (4 months ago) <Robert Vollmert>
a03b4a79e - Handle SIGTERM by throwing an asynchronous exception (4 months ago) <Robert Vollmert>
0284bfbec - Use Process.withCreateProcess to ensure child cleanup (4 months ago) <Robert Vollmert>
67092dab6 - Use maybeExit in more places (4 months ago) <Robert Vollmert>
91fa33bd5 - Add changelog (5 months ago) <Robert Vollmert>
97f2c7b38 - Remove newly deprecated functions (5 months ago) <Robert Vollmert>
d5ae20cf9 - Deprecate obsolete functions (5 months ago) <Robert Vollmert>
9d33338c1 - manpage: use rawSystemProcAction (5 months ago) <Robert Vollmert>
3aa45a2d7 - LibV09: use rawSystemProcAction (5 months ago) <Robert Vollmert>
200478ef9 - SetupWrapper: replace runProcess' by rawSystemProc (5 months ago) <Robert Vollmert>
1969327c8 - Set delegate_ctlc for rawSystemStdInOut, too (5 months ago) <Robert Vollmert>
f972102ef - Default delegate_ctlc to True (5 months ago) <Robert Vollmert>
a5986e37f - Rework subprocess helpers (5 months ago) <Robert Vollmert>
eefe45d9f - Drop unused module Distribution.Client.Compat.Process (5 months ago) <Robert Vollmert>
592a48e33 - Disambiguate qualified imports (5 months ago) <Robert Vollmert>

@Mikolaj Mikolaj marked this pull request as draft July 25, 2022 15:57
@Mikolaj
Copy link
Member Author

Mikolaj commented Jul 25, 2022

OK, let's do that in yet another PR.

@Mikolaj
Copy link
Member Author

Mikolaj commented Jul 25, 2022

Tried it, at least not conflicts this time...

@Mikolaj
Copy link
Member Author

Mikolaj commented Jul 25, 2022

Done on #8321.

@Mikolaj
Copy link
Member Author

Mikolaj commented Jul 25, 2022

CI passed fine, but closing in favour of #8321.

@Mikolaj Mikolaj closed this Jul 25, 2022
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

Successfully merging this pull request may close these issues.

3 participants