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

Fix repl test for buildReadlineNoMarkdown #11167

Merged
merged 11 commits into from
Jul 26, 2024
Merged

Fix repl test for buildReadlineNoMarkdown #11167

merged 11 commits into from
Jul 26, 2024

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
src/libcmd/markdown.cc Outdated Show resolved Hide resolved
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
23 checks passed
@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