-
-
Notifications
You must be signed in to change notification settings - Fork 321
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
WIP: Fix ninja warnings/exceptions #4427
Conversation
Try to normalize errors/warnings. One warning which was just a bare class instantition (thus, a no-op) now warns; two failures which used to raise SConsWarning now raise ImportError since that's what they are (has no visible effect to the user); another now raises UserError, as it is raised for an error in SConscripts. Fixes SCons#4036 Signed-off-by: Mats Wichmann <[email protected]>
Ah. This fails tests, because there are places where the warning (that wasn't previously raised) is triggered, and since it didn't happen before, the tests are not written to expect it and now show unexpected output. Will need to update the tests (or drop the change).
|
That's more interesting than I expected - looks like this is an internal action - it comes from the
|
Flagging this WIP/draft until some questions are answered. |
Signed-off-by: Mats Wichmann <[email protected]>
@mwichmann Is this still viable, or should it be closed? |
Well, the state hasn't changed: ninja (mildly) misuses some stuff, and "fixing" it causes some tests to go awry, making the change a little bigger than I was up for at the time, with possibly no real benefit. I can possibly be talking into dropping it, since I've never gone back to do the "further investigation". |
I'll revisit this someday, no need to have it hanging around here for now. |
Try to normalize errors/warnings. One warning which was just a bare class instantition (thus, a no-op) now warns; two failures which used to raise
SConsWarning
now raiseImportError
since that's what they are (has no visible effect to the user); another now raisesUserError
, as it is raised for an error in SConscripts.There are no API or documentation impacts of this change, just consistency.
Fixes #4036
Contributor Checklist:
CHANGES.txt
(and read theREADME.rst
)