`sage-logger`: Suppress "No such file or directory" messages #37785

mkoeppe · 2024-04-11T06:06:14Z

A minor follow-up after #37391:

Seen for example in
https://github.com/sagemath/sage/actions/runs/8626379356/job/23644513913?pr=37570#step:11:3227:

#24 4091.8 /sage/build/bin/sage-logger: line 66: /usr/bin/time: No such file or directory
#24 4091.8 [sagemath_doc_html-none] installing. Log file: /sage/logs/pkgs/sagemath_doc_html-none.log
#24 5807.5 cat: /sage/logs/pkgs/sagemath_doc_html-none.time: No such file or directory

We suppress these (harmless) messages which show up when /usr/bin/time is not available.

📝 Checklist

The title is concise and informative.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

…me is not available

github-actions · 2024-04-11T06:47:20Z

Documentation preview for this PR (built with commit ededb59; changes) is ready! 🎉

DavidAyotte

It looks good to me!

mkoeppe · 2024-04-13T23:30:55Z

Thanks!

tornaria · 2024-04-13T23:56:42Z

build/bin/sage-logger

@@ -63,15 +63,15 @@ fi

 timefile="$logdir/$logname.time"
 rm -f "$timefile"
-if /usr/bin/time -h -o /dev/null true; then
+if /usr/bin/time -h -o /dev/null true 2>/dev/null; then


I'm sure ubuntu has /usr/bin/time available. The problem is that you are using nonstandard arguments to time(1):

$ /usr/bin/time -h -o /dev/null /usr/bin/time: invalid option -- 'h' Try '/usr/bin/time --help' for more information. $ echo $? 125

Also, if you want to parse the time you should pass -p to ensure the standard format is used (https://www.man7.org/linux/man-pages/man1/time.1p.html), and then you can just use the bash builtin.

For what you are trying to acomplish, you could also use bash variables $EPOCHSECONDS (or $EPOCHREALTIME for more precision).

The reviewer specifically asked for the human-readable format to be used.
The whole thing is just a convenience.

The reviewer specifically asked for the human-readable format to be used. The whole thing is just a convenience.

I don't know what is "the human-readable format" but -h is not supported by gnu time(1) nor by the bash builtin.

Gnu time does support -o but not -h.

If you want to bypass the bash builtin, the better approach is to use \time which will still search in PATH.

For what you are trying to acomplish, you could also use bash variables $EPOCHSECONDS (or $EPOCHREALTIME for more precision).

There's also $SECONDS.

The reviewer specifically asked for the human-readable format to be used. The whole thing is just a convenience.

I see, but then the bash builtin time is just what the reviewer asked for.

You could

do the redirect 2>&1 inside the sh -c "..."

capture with 2> the output of time

(since the bash builtin doesn't support -o)

If you want to prepare a follow-up PR, I can test it for its portability. I don't have time to work on it myself at the moment.

If you want to prepare a follow-up PR, I can test it for its portability. I don't have time to work on it myself at the moment.

Please don't tell me what I should do with my time.

My comment before was just remarking that the current PR does not fix the issue but only hides it. Reverting cb4e718 for the time being seems to be easier workaround, until you or someone else gets the time to improve the output in a portable way.

Please don't tell me what I should do with my time.

@tornaria And I am not, so don't tell me what I shouldn't tell you.

tornaria · 2024-04-16T19:12:30Z

Please revert cb4e718 so this works on all supported platforms.

mkoeppe · 2024-04-16T19:14:31Z

No, that's outside of the scope of this PR.

mkoeppe · 2024-04-16T19:15:17Z

Per PR description, this PR resolves the messages displayed /usr/bin/time: No such file or directory when there is no /usr/bin/time binary.

tornaria · 2024-04-16T22:24:43Z

No, that's outside of the scope of this PR.

It's in scope for #37391 which was incorrect in this regard. The follow-up should be fixing, not hiding it.

The error you showed is

cat: /sage/logs/pkgs/sagemath_doc_html-none.time: No such file or directory

this has nothing to do with the absence of /usr/bin/time, rather with the fact that time -h fails because of the non-standard option.

Reverting cb4e718 seems easy enough; this ends up with your original PR which is the one I assume you tested most. Trying to satisfy a very reasonable request by @kwankyu is great, but not at the expense of breaking in a lot of supported systems.

mkoeppe · 2024-04-16T23:31:09Z

Please, Gonzalo, the PR author sets scope of the PR.

mkoeppe · 2024-04-16T23:32:44Z

If you look at the actual ticket description, line 1:

#24 4091.8 /sage/build/bin/sage-logger: line 66: /usr/bin/time: No such file or directory
#24 4091.8 [sagemath_doc_html-none] installing. Log file: /sage/logs/pkgs/sagemath_doc_html-none.log
#24 5807.5 cat: /sage/logs/pkgs/sagemath_doc_html-none.time: No such file or directory

So yes, this PR is about absence of /usr/bin/time as indicated.

tornaria · 2024-04-17T01:21:33Z

If you look at the actual ticket description, line 1:

#24 4091.8 /sage/build/bin/sage-logger: line 66: /usr/bin/time: No such file or directory
#24 4091.8 [sagemath_doc_html-none] installing. Log file: /sage/logs/pkgs/sagemath_doc_html-none.log
#24 5807.5 cat: /sage/logs/pkgs/sagemath_doc_html-none.time: No such file or directory

So yes, this PR is about absence of /usr/bin/time as indicated.

But #37391 is also broken on boxes where /usr/bin/time is available but

$ /usr/bin/time -h -o /dev/null true
/usr/bin/time: invalid option -- 'h'
Try '/usr/bin/time --help' for more information.
$ echo $?
125

This includes most linux boxes.

I grant you that changing this to use the builtin time or $SECONDS is more work, so I understand not doing that. But changing this to not use -h seems very easy.

I'll show myself the exit. I was only trying to help since I know (and appreciate) you are always concerned about portability.

PS: note that time bash builtin output cannot be captured:

$ time 2> /dev/null

real	0m0.000s
user	0m0.000s
sys	0m0.000s
$

so the obvious using time or time -p and capture with 2> doesn't seem to work. What you can do is run the time in a subshell like so:

$ ( time true ) 2> /dev/null
$

In fact, you could do something like the following:

$ cmd="echo some stdout; sleep 1; echo and some stderr; exit 42"
$ timeit=$( (>>logfile.txt 2>&1 time sh -c "$cmd") 2>&1 ) ; status=$?
$ echo $status
42
$ echo $timeit
real 0m1.004s user 0m0.003s sys 0m0.002s
$ cat logfile.txt 
some stdout
and some stderr
$

PS2: when you want to skip a bash builtin, the common idiom is \time which will still search for time in PATH so you avoid hardcoding it and allow the user to replace the command if they so wish.

mkoeppe · 2024-04-17T01:23:31Z

I'll show myself the exit. I was only trying to help

That's appreciated, and I will benefit from the details and suggestions that you made when I work on the follow-up PR. Thank you.

tornaria · 2024-04-17T01:40:32Z

I'll show myself the exit. I was only trying to help

That's appreciated, and I will benefit from the details and suggestions that you made when I work on the follow-up PR. Thank you.

I think my last suggestion might be useful. It seems to me that the format for the bash builtin time is similar to the output of time -h that you parse, so possibly you don't need to change report_time.

Since all of our scripts already depend on bash (as the shebang shows) depending on bashisms of the time builtin is "kind of" ok (at least it should be the same everywhere).

… messages       A minor follow-up after sagemath#37391: Seen for example in https://github.com/sagemath/sage/actions/runs/8626379356/job/23644513913 ?pr=37570#step:11:3227: ``` sagemath#24 4091.8 /sage/build/bin/sage-logger: line 66: /usr/bin/time: No such file or directory sagemath#24 4091.8 [sagemath_doc_html-none] installing. Log file: /sage/logs/pkgs/sagemath_doc_html-none.log sagemath#24 5807.5 cat: /sage/logs/pkgs/sagemath_doc_html-none.time: No such file or directory ``` We suppress these (harmless) messages which show up when `/usr/bin/time` is not available. ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies    URL: sagemath#37785 Reported by: Matthias Köppe Reviewer(s): David Ayotte, Gonzalo Tornaría, Matthias Köppe

sagemathgh-37840: `sage-logger`: Replace use of `/usr/bin/time` by bash keyword `time`       Follow-up after - sagemath#37391 - sagemath#37785 ... to make the display of package build times available on Linux too. Fixes sagemath#37785 (comment), closes sagemath#37832. ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    - Depends on sagemath#37785 URL: sagemath#37840 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee

sagemathgh-37840: `sage-logger`: Replace use of `/usr/bin/time` by bash keyword `time`       Follow-up after - sagemath#37391 - sagemath#37785 ... to make the display of package build times available on Linux too. Fixes sagemath#37785 (comment), closes sagemath#37832. ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    - Depends on sagemath#37785 URL: sagemath#37840 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe

build/bin/sage-logger: Suppress error message output when /usr/bin/ti…

ededb59

…me is not available

DavidAyotte approved these changes Apr 13, 2024

View reviewed changes

DavidAyotte added s: positive review and removed s: needs review labels Apr 13, 2024

tornaria reviewed Apr 13, 2024

View reviewed changes

tornaria added the s: needs work label Apr 16, 2024

dimpase added s: needs review and removed s: positive review labels Apr 19, 2024

dimpase mentioned this pull request Apr 19, 2024

/usr/bin/time should be a pre-req #37832

Closed

vbraun merged commit e7cf269 into sagemath:develop Apr 27, 2024

github-actions bot removed the s: positive review label Apr 27, 2024

Uh oh!

sage-logger: Suppress "No such file or directory" messages #37785

sage-logger: Suppress "No such file or directory" messages #37785

Uh oh!

Conversation

mkoeppe commented Apr 11, 2024

📝 Checklist

⌛ Dependencies

Uh oh!

github-actions bot commented Apr 11, 2024

Uh oh!

DavidAyotte left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Apr 13, 2024

Uh oh!

tornaria Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

mkoeppe Apr 14, 2024

Choose a reason for hiding this comment

Uh oh!

tornaria Apr 14, 2024

Choose a reason for hiding this comment

Uh oh!

tornaria Apr 14, 2024

Choose a reason for hiding this comment

Uh oh!

mkoeppe Apr 14, 2024

Choose a reason for hiding this comment

Uh oh!

tornaria Apr 14, 2024

Choose a reason for hiding this comment

Uh oh!

mkoeppe Apr 14, 2024

Choose a reason for hiding this comment

Uh oh!

tornaria commented Apr 16, 2024

Uh oh!

mkoeppe commented Apr 16, 2024

Uh oh!

mkoeppe commented Apr 16, 2024

Uh oh!

tornaria commented Apr 16, 2024

Uh oh!

mkoeppe commented Apr 16, 2024

Uh oh!

mkoeppe commented Apr 16, 2024

Uh oh!

tornaria commented Apr 17, 2024

Uh oh!

mkoeppe commented Apr 17, 2024

Uh oh!

tornaria commented Apr 17, 2024

Uh oh!

Uh oh!

`sage-logger`: Suppress "No such file or directory" messages #37785

`sage-logger`: Suppress "No such file or directory" messages #37785