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

[shell] improve mawk detection #3463

Merged
merged 6 commits into from
Oct 7, 2023

Conversation

step-
Copy link
Contributor

@step- step- commented Oct 6, 2023

The version check bash code relies on the following mawk source code, extracted from mawk 1.3.4 20230322.

version.c:
18-  #include "init.h"
19-  #include "patchlev.h"
20-
21:  #define	 VERSION_STRING	 \
22-    "mawk %d.%d%s %s\n\
23-  Copyright 2008-2022,2023, Thomas E. Dickey\n\
24-  Copyright 1991-1996,2014, Michael D. Brennan\n\n"
....
30-  void
31-  print_version(FILE *fp)
32-  {
33:      fprintf(fp, VERSION_STRING, PATCH_BASE, PATCH_LEVEL, PATCH_STRING, DATE_STRING);
34-      fflush(fp);
35-
36-  #define SHOW_RANDOM "random-funcs:"

patchlev.h:
13-  /*
14-   * $MawkId: patchlev.h,v 1.128 2023/03/23 00:23:57 tom Exp $
15-   */
16:  #define  PATCH_BASE	1
17-  #define  PATCH_LEVEL	3
18-  #define  PATCH_STRING	".4"
19-  #define  DATE_STRING    "20230322"

* Use the all-compatible mawk `-W version` option.
  junegunn#3313 (comment).
* Do not remap the history key if no dependent commands is installed
  (perl, awk or mawk in this order).
* Run the command and not a function consistently with junegunn#3462.

The version check bash code relies on the following mawk source code,
extracted from mawk 1.3.4 20230322.

```
version.c:
18-  #include "init.h"
19-  #include "patchlev.h"
20-
21:  #define	 VERSION_STRING	 \
22-    "mawk %d.%d%s %s\n\
23-  Copyright 2008-2022,2023, Thomas E. Dickey\n\
24-  Copyright 1991-1996,2014, Michael D. Brennan\n\n"
....
30-  void
31-  print_version(FILE *fp)
32-  {
33:      fprintf(fp, VERSION_STRING, PATCH_BASE, PATCH_LEVEL, PATCH_STRING, DATE_STRING);
34-      fflush(fp);
35-
36-  #define SHOW_RANDOM "random-funcs:"

patchlev.h:
13-  /*
14-   * $MawkId: patchlev.h,v 1.128 2023/03/23 00:23:57 tom Exp $
15-   */
16:  #define  PATCH_BASE	1
17-  #define  PATCH_LEVEL	3
18-  #define  PATCH_STRING	".4"
19-  #define  DATE_STRING    "20230322"
```
@calestyo
Copy link
Contributor

calestyo commented Oct 6, 2023

Other than the comments made in the code above, I think it looks mostly good, I especially like that you saved at least one execution of awk, though:

  • I'd split it up in more commits, you do now various mostly unrelated things in one commit, adding the command, fixing the version check, changing the output redirection (which I would however drop completely), make the code handle the situation when both perl and [m]awk aren't available.
    I think it's easier to understand and follow in smaller pieces
  • Handling the case that awk isn't available is in principle fine by me, though at least POSIX wise, systems are expected to have awk. So assuming POSIX systems, handling this is unnecessary.
  • btw: I think the (x *1000 +y) *1000 +z >= 1003004 part is at least a bit fragile, should e.g. ever a big minor version number be used. Also you still have the check for the date, so apart from me not liking such a runtime check in general, t should be fine.

I checked it on Debian with mawk 1.3.4.20200120-3.1, which yields __fzf_awk=awk, and with 1.3.4.20230808-1, which yields __fzf_awk=mawk. So that works :-)

shell/key-bindings.bash Outdated Show resolved Hide resolved
@step-
Copy link
Contributor Author

step- commented Oct 7, 2023

@calestyo thank you for testing on Debian.
@junegunn you're right, who can be sure how all target platforms and bash versions will behave when stdout is closed. Better be safe.

Copy link
Owner

@junegunn junegunn left a comment

Choose a reason for hiding this comment

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

Trying to remove unnecessary diffs.

shell/key-bindings.bash Outdated Show resolved Hide resolved
shell/key-bindings.bash Outdated Show resolved Hide resolved
shell/key-bindings.bash Outdated Show resolved Hide resolved
@junegunn
Copy link
Owner

junegunn commented Oct 7, 2023

  • Handling the case that awk isn't available is in principle fine by me, though at least POSIX wise, systems are expected to have awk. So assuming POSIX systems, handling this is unnecessary.

I also think it's unnecessary. The absence of awk can be a sign of a broken system, and we should probably warn the user with an error instead of silently disabling a subset of our feature, making it harder to fix the system.

shell/key-bindings.bash Outdated Show resolved Hide resolved
shell/key-bindings.bash Outdated Show resolved Hide resolved
shell/key-bindings.bash Outdated Show resolved Hide resolved
@junegunn junegunn merged commit 0f15f1a into junegunn:master Oct 7, 2023
5 checks passed
@junegunn
Copy link
Owner

junegunn commented Oct 7, 2023

Merged, thanks!

@calestyo
Copy link
Contributor

calestyo commented Oct 8, 2023

Looks good to me, though I probably still have placed the command inside the variable, as described above.

@calestyo
Copy link
Contributor

calestyo commented Oct 8, 2023

Oh, one more thing that I had noticed before, but forgot to write down:

IFS=' .' read n x y z d <<< $(command mawk -W version 2> /dev/null)

Wouldn't it be safer to use read -r?

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