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

Change option --color choices to auto/always/never #1701

Merged
merged 13 commits into from Sep 5, 2016

Conversation

riccardomurri
Copy link
Contributor

@riccardomurri riccardomurri commented Mar 25, 2016

The option values are as in ls --color and grep --color,
except that "auto" is the default which colorizes logs and output
whenever EB's STDOUT is connected to a terminal.

Note that this requires PR hpcugent/vsc-base#225 to be merged!
(Otherwise fancylogger.logToScreen() does not have a color parameter.)

The option values are as in `ls --color` and `grep --color`,
except that "auto" is the default which colorizes logs and output
whenever EB's STDOUT is connected to a terminal.
@hpcugentbot
Copy link
Contributor

Automatic reply from Jenkins: Can I test this?

… be used.

This is necessary after the recent API changes in PR easybuilders#225 on
`vsc-base`: now the colorization options in `fancylogger` use a
three-value enum `auto`/`always`/`never`.
…ly` is not available.

We detect if STDOUT/STDERR is connected to a terminal and then just
assume that everyone is using an ANSI-compatible one these days...
@@ -112,7 +112,8 @@ def find_rel_test():
zip_safe=False,
install_requires=[
'setuptools >= 0.6',
"vsc-base >= 2.4.18",
"vsc-base >= 2.5.0",
'humanfriendly', # determine whether terminal supports ANSI color
Copy link
Member

Choose a reason for hiding this comment

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

this should be moved down into extras_require, only the packages that are strictly required to make eb work should be listed in install_requires

@boegel boegel added this to the v2.8.0 milestone Mar 25, 2016
@boegel
Copy link
Member

boegel commented Mar 25, 2016

Jenkins: ok to test

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2912/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@@ -208,7 +233,8 @@ def main(args=None, logfile=None, do_build=None, testing=False):

# initialise logging for main
global _log
_log, logfile = init_logging(logfile, logtostdout=options.logtostdout, silent=testing or options.terse)
_log, logfile = init_logging(logfile, logtostdout=options.logtostdout,
silent=(testing or options.terse)), color=options.color)
Copy link
Member

Choose a reason for hiding this comment

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

the )) here should be ), syntax error

@riccardomurri you didn't test this, did you? Did you?! ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Kenneth Hoste, Fri, Mar 25, 2016 at 04:07:15PM -0700:)

@@ -208,7 +233,8 @@ def main(args=None, logfile=None, do_build=None, testing=False):

 # initialise logging for main
 global _log
  • _log, logfile = init_logging(logfile, logtostdout=options.logtostdout, silent=testing or options.terse)
  • _log, logfile = init_logging(logfile, logtostdout=options.logtostdout,
  •                             silent=(testing or options.terse)), color=options.color)
    

the )) here should be ), syntax error

@riccardomurri you didn't test this, did you? Did you?! ;-)

Damn Jenkins, you caught me!

@pforai
Copy link
Contributor

pforai commented Mar 26, 2016

+1 for this!

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2918/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2919/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

for their meaning.
"""
# turn color=auto/yes/no into a boolean value
if options.color == 'auto':
Copy link
Member

Choose a reason for hiding this comment

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

@riccardomurri this should use colorize rather than options.color?

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2920/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@riccardomurri
Copy link
Contributor Author

@boegel I cannot reproduce the failure that Jenkins reported on my laptop, and I cannot understand what part of the code has actually failed from the traceback.... help?

@boegel
Copy link
Member

boegel commented Mar 29, 2016

@riccardomurri The failing test is probably because the PR is not being tested against the updated vsc-base, i.e. fancylogger.logToScreen doesn't consume a named argument colorize yet

In other words: #225 needs to be merged first.

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2921/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@boegel boegel removed this from the v2.8.0 milestone May 10, 2016
@boegel boegel modified the milestones: v2.x, v2.8.0 May 10, 2016
@boegel
Copy link
Member

boegel commented Sep 2, 2016

@riccardomurri please merge riccardomurri#26 to sync this with current develop

sync with develop & resolve conflicts in main.py
@riccardomurri
Copy link
Contributor Author

@boegel done.

@boegel boegel modified the milestones: v2.9.0, v2.x Sep 2, 2016
@@ -126,6 +126,11 @@ def find_rel_test():
],
extras_require = {
'yeb': ["PyYAML >= 3.11"],
'coloredlogs': [
'vsc-base[coloredlogs] >= 2.5.0',
Copy link
Member

Choose a reason for hiding this comment

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

@riccardomurri please bump to 2.5.3 as minimal required version

Version 2.5.3 of `vsc-base` adds log colorization support.
@riccardomurri
Copy link
Contributor Author

@boegel bumped vsc-base requirement in commit 544563f

@boegel
Copy link
Member

boegel commented Sep 5, 2016

lgtm, thanks @riccardomurri!

@boegel boegel merged commit 6a519d8 into easybuilders:develop Sep 5, 2016
@riccardomurri riccardomurri deleted the coloredlogs branch September 5, 2016 11:53
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.

None yet

4 participants