-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add EMCC_LOGGING=0 to disable all logging #19531
Conversation
@@ -29,9 +29,14 @@ | |||
# Configure logging before importing any other local modules so even | |||
# log message during import are shown as expected. | |||
DEBUG = int(os.environ.get('EMCC_DEBUG', '0')) | |||
EMCC_LOGGING = int(os.environ.get('EMCC_LOGGING', '1')) | |||
log_level = logging.ERROR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be WARN by default? (what was the default before? do we actually use WARN?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EMCC_LOGGING default to 1, so the default here is, and was before, logging.INFO
.
logging.ERROR
is the new level introduced here that is only used if you explicitly set EMCC_LOGGING=0
.
Having a way to disable/reduce logging SGTM. I think I agree with @juj that it's nice to get logs when system libraries are rebuilt but not useful to hear about the symbols files. I wondered whether that distinction was useful for users, but I think having the library building output by default is probably still useful, because users shouldn't see it very often (only the first time they build in a new unusual configuration, and when they update their toolchain). |
I think this change is needed/useful reagardless of how we solve the "some caching messages are interesting and other are not".. since we have had users who really an truly want zero stderr from emcc. (For example, parsing the output of |
Oh yes, I agree about that. And I guess even if we adjust the logging levels in the future (e.g. allow different numbers or something), this change is pretty backward compatible since right now 0 is the only value that does anything. |
@@ -7118,6 +7118,7 @@ def test_realpath_2(self): | |||
Resolved: "/" => "/" | |||
''', self.run_js('a.out.js')) | |||
|
|||
@with_env_modify({'EMCC_LOGGING': '0'}) # this test assumes no emcc output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit odd to me. It disables logging, but the test wants to check that there is no problem that causes logging. So it is really a test for the flag disabling logging, but not for the problems that can cause logging existing or not.
In practice most of those warnings are from clang, which EMCC_LOGGING
doesn't handle. So I'm not too worried here in practice, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EMCC_LOGGING
only effect logging.. which is completely separate to the warnings system. EMCC_LOGGING
does not effect warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That's less worrying to me then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps its a little confusing... that emscripten uses both python's logging module and also its own "diagnostics.py" module?
This can be useful when running tests that want to check the stderr of emscripten.
This was suggested as an aternative to #19085 and should fix #18607 but maybe isn't quite right to solve #18622.
Fixes: #19085