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

rework libeinfo #651

Closed
wants to merge 10 commits into from
Closed

rework libeinfo #651

wants to merge 10 commits into from

Conversation

xxc3nsoredxx
Copy link
Contributor

Rework how libeinfo does curses stuff. This is not by any means ready to merge as-is. For example, OpenRC core has not been changed where needed (places where ecolor is called are the most obvious ones). There also isn't a non-curses fallback yet, but that should be easy to add once everything else is satisfactory. Not to mention squashing/rewording commits.

This is just to get some feedback on what I currently have.

See also: #619

@xxc3nsoredxx
Copy link
Contributor Author

Squashed a few of the commits/cleaned up the messages.

@williamh
Copy link
Contributor

We don't need command line equivalents for ewarnx and eerrorx.
In a script, you can use ewarn then exit or do the same with eerror.

Uses (n)curses/terminfo for "fancy" stuff. No env vars yet.

These functions are not sent to syslog (yet):
- ewarn
- ewarnx
- eerror
- eerrorx

The syslog id for elog(3) is currently hardcoded.

Some intentional API/behavior differences:
- ebracket(3) does the same negative column stuff as described in
  _move_col(). Pretty sick if you ask me.

- the length of the "prefix|" string is counted as part of the total
  length returned by einfo(3) et al.
Properly implement is_quiet(), is_really_quiet(), and is_verbose() by
checking the truthyness of the EINFO_QUIET, EERROR_QUIET, EINFO_VERBOSE
env vars, respectively.

Use the EINFO_LOG env var to set the log id for syslog messages and send
ewarn(3), ewarnx(3), eerror(3), and eerrorx(3) to syslog. Only happens
if the env var is set tho.

Behavior change for EINFO_INDENT:
- integer overflow is treated as valid, but >INDENT_MAX (which it
  arguably is), and therefore chomped to INDENT_MAX

API/behavior changes for EINFO_COLOR:
- case insensitive on color names
- more predictable error case (keep current on parse error)
- use the last (valid) definition if multiple given for a single color
Used to ensure that the terminal is actually good for curses use, and
enables short circuiting functions that do curses stuff in case the
terminal is not.
Replaced with an array and two calls to tput(s). No need for "proper
string concatenation" if there is no string concatenation :P
curses/terminfo errors are "basically handled" through:

- tiparm() returning NULL on error
- tputs() returning an error if the string is NULL

We don't actually do anything with the tputs() error since all functions
that call it are void.
The rewrite should be "complete" as far as matching features goes. Still
gotta do some small TODO's. As well as non-curses.
Set the message in the brackets to "..." if the message is empty. Only
possible with ebracket().
Ensure fmt arg is checked at the call site.
@williamh
Copy link
Contributor

Please rebase on master.

I also see a couple of changes.

On line 50 of einfo.h:

__attribute__((__deprecated__("this shit is gone yo")))

What does this apply to? Also, can you fix the message to be something like "this is no longer used"?

Please update if statements that just have one statement in the if or else blocks to not use curly braces around the statement:

if (foo) {
    bar;
}

should be:

if (foo)
    bar;

Can you also remove the vim mode lines?

Thanks,

William

@xxc3nsoredxx
Copy link
Contributor Author

I can do the single-line if thing, sure. The __deprecated__ message is currently just a placeholder to alert me to things which use the ecolor() function in OpenRC and will be changed before I un-draft the PR. Same with removing the mode line.

Copy link
Member

@vapier vapier left a comment

Choose a reason for hiding this comment

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

there's a lot of commits in this stack. will prob need to pull out some of the smaller ones independently.

@@ -47,6 +47,7 @@ typedef enum
} ECOLOR;

/*! @brief Returns the ASCII code for the color */
__attribute__((__deprecated__("this shit is gone yo")))
Copy link
Member

Choose a reason for hiding this comment

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

sorry, but we can't accept things like this. ignoring the puerile nature, it's not helpful or clear to anyone who hits/sees this warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was merely meant as a message while I was working and not for the final cleaned up/squashed commits.

@vapier
Copy link
Member

vapier commented Jan 5, 2024

there's a lot of commits in this stack. will prob need to pull out some of the smaller ones independently.

ok, i flipped through the patch series now, and it doesn't make sense to pull things out. this is a PR showing progress of implementing a new libeinfo library and would make sense squashing all into one.

i'm inclined to go with my half-baked idea that i mentioned in the open issue. since we already support building openrc w/out termcap (curses) support, we should lean into that: delete support for termcap entirely and just require an ANSI compliant terminal.

@xxc3nsoredxx
Copy link
Contributor Author

I can close this PR if you instead want to just drop curses and require an ANSI terminal.

@vapier
Copy link
Member

vapier commented Feb 18, 2024

you want to send a PR to delete curses support ?

@xxc3nsoredxx
Copy link
Contributor Author

Sure, I think I can do that.

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