search

blackberry / BB10-Webworks-Packager

The BB10 WebWorks Packager bundles the App content with the BB10 WebWorks Framework to create a BAR to run on the BB10 Device (or simulator)
27 stars 18 forks source link

Add log level params to bbwp #227

Closed gtanner closed 11 years ago

gtanner commented 11 years ago

Fixes Issue #221

This fix was added mainly for phonegap where a lot of the warnings being shown were just noise and causing confusion.

Three commands now exist: --verbose: the default and the current behavour of bbwp. logs everything --warn: logs warning and info messages --error: logs error and info messages

Not providing any of these args sets the level to verbose.

When providing these on the command line, the most verbose wins. So calling bbwp --warn --error will set the level to warn.

The reason we always log info messages is because these messages are mostly used for important information such as (build done, build starting, etc).

nukulb commented 11 years ago

@jkeshavarzi want to give a quick review?

gtanner commented 11 years ago

This is kinda awkward when you look at it now, we should really audit some of our info level logs and make them always appear.

ie: add a log method to the logger that logs always switch over some of the more important messages to the new method. (build start, build finished)

nukulb commented 11 years ago

FYI - @gtanner @jkeshavarzi The cut off is tomorrow for the next release (final respin) going out sometime soon (exact dates undisclosed) If this is not tested/reviewed etc.. it will have to wait till January release to make it public.

@jkeshavarzi can you test it enough in the next little bit?

jamesjhedley commented 11 years ago

And what about the non-important messages? I agree it's getting rather weird now. --verbose and --warning essentially do the same thing now.

jamesjhedley commented 11 years ago

@nukulb That depends on the complexity of the logging.

nukulb commented 11 years ago

If the current change is not good enough perhaps this should wait till the next release.

jamesjhedley commented 11 years ago

Not sure what the best solution is at the moment, but the way it is now, definitely feels weird. Having a --verbose option for instance, when we essentially verbose by default and the --warn and --error flags are a bit confusing, given the --warn actually does warnings and errors.

Might be best to figure out exactly what we want to do here and get it in for the Jan release.

nukulb commented 11 years ago

I will be fine with leaving it out for this release, I would like to hear from @gtanner on what he thinks the affect is on cordova devs.

kwallis commented 11 years ago

What I have seen in the past is increasing levels of logging. Error would only contain errors, warning would contain warnings and errors, info would contain all, for example.

From: James Keshavarzi Sent: Tuesday, 4 December, 2012 5:04:31 PM EDT To: blackberry/BB10-Webworks-Packager Reply To: blackberry/BB10-Webworks-Packager Subject: Re: [BB10-Webworks-Packager] Add log level params to bbwp (#227)

Not sure what the best solution is at the moment, but the way it is now, definitely feels weird. Having a --verbose option for instance, when we essentially verbose by default and the --warn and --error flags are a bit confusing, given the --warn actually does warnings and errors.

Might be best to figure out exactly what we want to do here and get it in for the Jan release.

\u2014 Reply to this email directly or view it on GitHubhttps://github.com/blackberry/BB10-Webworks-Packager/pull/227#issuecomment-11017802.

This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

gtanner commented 11 years ago

I think what is here is fine to ship, all I want is --error for Cordova.

Lets ship as is and clean up verbose logging for jan.

nukulb commented 11 years ago

@kwallis cc

kwallis commented 11 years ago

I really feel like the command line should be something like:

--loglevel error | warn | info

gtanner commented 11 years ago

@kwallis I was going to do it that way but you guys already had a --verbose command.

kwallis commented 11 years ago

I am not above deprecating --verbose. ;)

This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

gtanner commented 11 years ago

:+1: I am all for doing it that way now that I know that --verbose does nothing

nukulb commented 11 years ago

What does —verbose even do currently?

From: Ken Wallis notifications@github.com<mailto:notifications@github.com> Reply-To: blackberry/BB10-Webworks-Packager reply@reply.github.com<mailto:reply@reply.github.com> Date: Wed, 5 Dec 2012 12:47:02 -0800 To: blackberry/BB10-Webworks-Packager BB10-Webworks-Packager@noreply.github.com<mailto:BB10-Webworks-Packager@noreply.github.com> Cc: Nukul Bhasin nbhasin@rim.com<mailto:nbhasin@rim.com> Subject: Re: [BB10-Webworks-Packager] Add log level params to bbwp (#227)

I am not above deprecating --verbose. ;)

This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

— Reply to this email directly or view it on GitHubhttps://github.com/blackberry/BB10-Webworks-Packager/pull/227#issuecomment-11059738.

This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

gtanner commented 11 years ago

@nukulb nothing

(please delete message content if you are using email, we don't need to have all the history of the thread in your reply :P)

gtanner commented 11 years ago

@kwallis and @nukulb

please see updated commit, output will now look like:

load-device:
 [exec] [BUILD]   Populating application source
 [exec] [WARN]    webworks.js is now packaged as local:///chrome/webworks.js
 [exec] [BUILD]   Parsing config.xml
 [exec] [WARN]    Failed to find feature with id: blackberry.find
 [exec] [WARN]    Failed to find feature with id: blackberry.identity.phone
 [exec] [WARN]    Failed to find feature with id: blackberry.pim.Address
 [exec] [WARN]    Failed to find feature with id: blackberry.pim.Contact
 [exec] [WARN]    Failed to find feature with id: blackberry.io.file
 [exec] [WARN]    Failed to find feature with id: blackberry.utils
 [exec] [WARN]    Failed to find feature with id: blackberry.io.dir
 [exec] [WARN]    Failed to find feature with id: blackberry.app.event
 [exec] [WARN]    Failed to find feature with id: blackberry.system.event
 [exec] [WARN]    Failed to find feature with id: blackberry.widgetcache
 [exec] [WARN]    Failed to find feature with id: blackberry.media.camera
 [exec] [WARN]    Failed to find feature with id: blackberry.media.microphone
 [exec] [BUILD]   Generating output files
 [exec] [INFO]    Info: Package created: /Users/gtanner/Projects/incubator-cordova-blackberry-webworks/example/build/simulator/cordovaExample.bar
 [exec] [INFO]    Info: Package created: /Users/gtanner/Projects/incubator-cordova-blackberry-webworks/example/build/device/cordovaExample.bar
 [exec] [INFO]    Info: Bar signed.
 [exec] [BUILD]   BAR packaging complete

(ignore the [exec] prefix as that is from the cordova build scripts)

nukulb commented 11 years ago

I am good with with the format, I like it.

kwallis commented 11 years ago

Looks good to me

jeffheifetz commented 11 years ago

Its weird that the info output has [INFO] and Info: for the text

gtanner commented 11 years ago

@jeffheifetz yes it does ;) It has always done that.

jamesjhedley commented 11 years ago

@jeffheifetz I believe those extra info: elements are coming from the native packager/blackberry-signer. We could always parse the output from these tools and remove this text, if we really dislike it.

nukulb commented 11 years ago

lets not worry about that.

jamesjhedley commented 11 years ago

r+

nukulb commented 11 years ago

@gtanner - can you push to the blackberry-webworks fork please?

Its a lot easier merge pull requests because CI is hooked up to that repo.

Let me know when you do and I can merge it quickly after that

nukulb commented 11 years ago

CI build failing due [INFO] lib/logger.js: line 31, col 19, Expected '!==' and instead saw '!='. [INFO] test/unit/lib/session.js: line 166, col 51, Missing space after ':'. [INFO] test/unit/lib/session.js: line 171, col 51, Missing space after ':'. [INFO] test/unit/lib/session.js: line 176, col 51, Missing space after ':'. [INFO] test/unit/lib/session.js: line 181, col 51, Missing space after ':'. [INFO] [INFO] 5 errors [INFO] [INFO] test/unit/lib/session.js : [INFO] Unused Variables: [INFO] result(165), result(170), result(175), result(180), [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------

gtanner commented 11 years ago

This is what I get locally:

gtanner:~/webworks/BB10-Webworks-Packager (log.level)$ jake lint
path.existsSync is now called `fs.existsSync`.

Pushed updates

nukulb commented 11 years ago

still getting these

test/unit/lib/session.js : Unused Variables: result(165), result(170), result(175), result(180),

What version of node are you on?

I assume 8.xxx ( latest) do you mind

most of us use 0.6.20 till we can we officially move (pull request is open)

npm install n

n module lets you switch easily between node versions, I am not sure if you have used it before.

gtanner commented 11 years ago

only 6 months behind ;)

pushed again

nukulb commented 11 years ago

it works now.

nukulb commented 11 years ago

merge conflicts :(

nukulb commented 11 years ago

resolved and merged, landed in next as 094db3deb8a3c986baac459ef0f8df26af4e3eac

please check it out and make sure no merges were resolved incorrectly.