Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion easybuild/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import sys
import tempfile
import traceback
from datetime import datetime

# IMPORTANT this has to be the first easybuild import as it customises the logging
# expect missing log output when this not the case!
Expand Down Expand Up @@ -84,6 +85,7 @@
from easybuild.tools.repository.repository import init_repository
from easybuild.tools.systemtools import check_easybuild_deps
from easybuild.tools.testing import create_test_report, overall_test_report, regtest, session_state
from easybuild.tools.utilities import time2str
from easybuild.tools.version import EASYBLOCKS_VERSION, FRAMEWORK_VERSION, UNKNOWN_EASYBLOCKS_VERSION
from easybuild.tools.version import different_major_versions

Expand Down Expand Up @@ -585,6 +587,7 @@ def process_eb_args(eb_args, eb_go, cfg_settings, modtool, testing, init_session
return True

# build software, will exit when errors occurs (except when testing)
start_time = datetime.now()
if not testing or (testing and do_build):
exit_on_failure = not (options.dump_test_report or options.upload_test_report)

Expand All @@ -601,7 +604,7 @@ def process_eb_args(eb_args, eb_go, cfg_settings, modtool, testing, init_session
success_msg = "Build succeeded "
if build_option('ignore_test_failure'):
success_msg += "(with --ignore-test-failure) "
success_msg += "for %s out of %s" % (correct_builds_cnt, len(ordered_ecs))
success_msg += f"for {correct_builds_cnt} out of {len(ordered_ecs)} in {time2str(datetime.now() - start_time)}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could use the same style as other time reporting eg for the steps of ... (took TIME)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this?

== Build succeeded for 5 out of 5
... (took 46 mins 13 secs)

Not sure of this format here could be confused for something else and it was easier. It would need a 2nd print_msg to have it match more closely the other messages:

== Build succeeded for 5 out of 5
== ... (took 46 mins 13 secs)

But no strong opinion, can change it if you think one is better (which one?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine also having it on one line, maybe we could do something like

Build succeeded for 5 out of 5 (total 46 mins 13 secs)

Also no strong opinion on it (could always be adjusted later) but agree it would be nice to have the total runtime of the build as well.

Maybe for a separate PR, but related to this it might also be worth to get a total CPU/node time for parallel jobs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

== Build succeeded for 5 out of 5 (total 46 mins 13 secs)

I was wondering how that is better than

== Build succeeded for 5 out of 5 in 46 mins 13 secs

but while that reads more fluently it has too many numbers next to each other to read in a quick glimpse and now I see better why you preferred the "...took" variant..

I'll add a colon too:

Suggested change
success_msg += f"for {correct_builds_cnt} out of {len(ordered_ecs)} in {time2str(datetime.now() - start_time)}"
success_msg += f"for {correct_builds_cnt} out of {len(ordered_ecs)} (total: {time2str(datetime.now() - start_time)})"


repo = init_repository(get_repository(), get_repositorypath())
repo.cleanup()
Expand Down