Skip to content

Fix repl test for buildReadlineNoMarkdown#11167

Merged
roberth merged 11 commits intomasterfrom
repl-test-rejiggle
Jul 26, 2024
Merged

Fix repl test for buildReadlineNoMarkdown#11167
roberth merged 11 commits intomasterfrom
repl-test-rejiggle

Conversation

@roberth
Copy link
Member

@roberth roberth commented Jul 23, 2024

Motivation

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@roberth roberth requested a review from edolstra as a code owner July 23, 2024 17:30
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jul 23, 2024
Slightly custom because the automated formatting messes up the
braced initializer with named fields.
@roberth roberth force-pushed the repl-test-rejiggle branch from b86a7f5 to f35d1a1 Compare July 23, 2024 23:03
roberth added 6 commits July 24, 2024 12:48
... as well as match buildReadlineNoMarkdown.

Unfortunately it doesn't support long inputs or multiline inputs
for now.
This needs to make better use of the interacter interface.
@roberth roberth force-pushed the repl-test-rejiggle branch from f35d1a1 to 7d4d34a Compare July 24, 2024 10:48
@github-actions github-actions bot added repl The Read Eval Print Loop, "nix repl" command and debugger fetching Networking with the outside (non-Nix) world, input locking labels Jul 24, 2024
@roberth
Copy link
Member Author

roberth commented Jul 24, 2024

The CI failure on e48e0cb is similar to #11144 (comment), so I've cherry-picked a commit that logs more before aborting.
I now see that it might be happening in the CI's Nix instead of the newly built Nix. hm..

* @note: This assumes that the logger is operational
*/
[[noreturn]]
void panic(std::string_view msg);
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer a macro similar to printError() and debug() that wraps fmt() so we can write stuff like

panic("bla %s", arg);

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to keep it somewhat simple, because it might have to perform in a somewhat broken process state, though probably it shouldn't even use the logger then, and just write to fd 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made them signal safe.

For non-critical errors, we can throw Error as we did.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if signal safety is worth pursuing, since it's really too hard to ensure that nothing ever calls malloc() or whatever. It's better to keep signal handlers trivial (e.g. they should only wake up a thread by writing to a file descriptor).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed any mention of that. The goal is just to be robust; signal safety would have been a nice property, but it doesn't have to be a goal.

*
* @note: This assumes that the logger is operational
*/
#define NIX_UNREACHABLE() (panic(__FILE__, __LINE__, __func__))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#define NIX_UNREACHABLE() (panic(__FILE__, __LINE__, __func__))
#define unreachable() (panic(__FILE__, __LINE__, __func__))

@roberth roberth force-pushed the repl-test-rejiggle branch from 04c54b5 to 3172e88 Compare July 24, 2024 14:54
@roberth roberth added this to the nix-2.24 milestone Jul 25, 2024
Plus one or two tweaks.
@roberth roberth force-pushed the repl-test-rejiggle branch from ba99ca0 to 55a654a Compare July 25, 2024 13:50
@roberth
Copy link
Member Author

roberth commented Jul 26, 2024

Merging because it's blocking

  • 2.24 release
  • investigation of the spurious aborts in CI

panic can be changed as needed / desired, no problem.

@roberth roberth merged commit 861bd10 into master Jul 26, 2024
@roberth roberth deleted the repl-test-rejiggle branch July 26, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fetching Networking with the outside (non-Nix) world, input locking repl The Read Eval Print Loop, "nix repl" command and debugger with-tests Issues related to testing. PRs with tests have some priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants