Skip to content

Makefile: Detect if we are using gnu time#5

Merged
bors[bot] merged 1 commit intomasterfrom
makefile_darwin
Mar 27, 2018
Merged

Makefile: Detect if we are using gnu time#5
bors[bot] merged 1 commit intomasterfrom
makefile_darwin

Conversation

@jmprusi
Copy link
Copy Markdown
Contributor

@jmprusi jmprusi commented Mar 26, 2018

Due to differences between bsd time and gnu time command,
the current makefile files to run under Darwin.

This commit disables writing "bench.txt" for non linux systems.

@unleashed
Copy link
Copy Markdown
Contributor

There are non-linux OSes running GNU time. How about using time --version and looking for the string GNU time? Also there might be workarounds to have this information (alone, not including anything else but timing info) in a bench text file, although not sure it is really worth exploring.

@mikz
Copy link
Copy Markdown
Contributor

mikz commented Mar 27, 2018

@unleashed then those users can contribute that feature, right? :) This is improving it significantly for macOS users. gnutime is not even part of coreutils on brew (that some people install to get stuff like readline). And time is provided by shell anyway in some cases (like zsh).

Copy link
Copy Markdown
Contributor

@mikz mikz left a comment

Choose a reason for hiding this comment

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

👍

@unleashed
Copy link
Copy Markdown
Contributor

It's not a feature, but doing this right: you want to avoid using GNU time's features if you don't have GNU time. We'd also be better off providing a similar behaviour if possible/easy instead of disabling it.

Here we want to store the timing data to be shown at the end of the process - so if time does not have -o on BSDs/Darwin it might still be possible to dup stderr for time and redirect stdout/stderr to the original streams for the executed processes, getting the same desired result (although probably a different output format from time).

Something like \command time 3>&2 4> bench.txt 2>&4 sh -c "$@ 2>&3" might work everywhere and then the only thing needed would be to check which of the other arguments are supported on BSDs/Darwin userland. IIRC some BSDs also supported formatting the output.

@mikz
Copy link
Copy Markdown
Contributor

mikz commented Mar 27, 2018

@unleashed but no one cares about this feature on Darwin. First person that comes and cares can do that.
Right now it does not work and this is a fix to make it work.

Due to differences between bsd time and gnu time command,
this commits uses a different strategy for saving the overall
time depending on the time command.
@jmprusi jmprusi changed the title Makefile: Use gnu-time only under linux Makefile: Detect if we are using gnu time Mar 27, 2018
@jmprusi
Copy link
Copy Markdown
Contributor Author

jmprusi commented Mar 27, 2018

@unleashed @mikz Updated the PR, please review, now we check if the time command supports --version (gnu) and choose between two different strategies.

shift # get rid of the '-c' supplied by make.

\time -o bench.txt -a -f "[%E] : $*" -- sh -c "$*"
if command time --version > /dev/null 2>&1
Copy link
Copy Markdown
Contributor

@mikz mikz Mar 27, 2018

Choose a reason for hiding this comment

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

Unfortunately this does not work in zsh.

$ command time --version
time: illegal option -- -
usage: time [-lp] command.

I'm for dropping this feature altogether and let people opt in by setting on SHELL instead of this being in the Makefile:

SHELL = $(PROJECT_PATH)/script/make_report_time.sh

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mikz this should be interpreted by /bin/sh not zsh, working fine for me, with zsh.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see! 👍

@unleashed
Copy link
Copy Markdown
Contributor

bors r+

bors Bot added a commit that referenced this pull request Mar 27, 2018
5: Makefile: Detect if we are using gnu time r=unleashed a=jmprusi

Due to differences between bsd time and gnu time command,
the current makefile files to run under Darwin.

This commit disables writing "bench.txt" for non linux systems.
@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Mar 27, 2018

Build succeeded

@bors bors Bot merged commit 69a786b into master Mar 27, 2018
@bors bors Bot deleted the makefile_darwin branch March 27, 2018 11:33
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