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.

Sign up for GitHub

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

Jump to bottom

ninja tool: using SCons warnings framework incorrectly #4036

Open
mwichmann opened this issue Oct 19, 2021 · 3 comments · May be fixed by #4427
Open

ninja tool: using SCons warnings framework incorrectly #4036

mwichmann opened this issue Oct 19, 2021 · 3 comments · May be fixed by #4427
Labels
Ninja

Comments

@mwichmann
Copy link
Collaborator

mwichmann commented Oct 19, 2021 •
edited

(Edited to update) When I revisited a long pending set of checker gripes about the use of warnings, saw there are two instances in the ninja tool where a warning class is instantiated, but then nothing is done with it - it's not assigned to anything, it's not directly raised. Two other instances directly raise it, meaning it's not really a "warning" - it's actually an exception, it just happens to be the SConsWarning exception, which doesn't really make a lot of sense.

Instantiated, unused:
https://github.com/SCons/scons/blob/120b2cd0b71363f63e8eb4898a44f1ba8a48af64/SCons/Tool/ninja/NinjaState.py#L810C1-L816C10

scons/SCons/Tool/ninja/__init__.py

Line 235 in 120b2cd

SCons.Warnings.SConsWarning("Generating multiple ninja files not supported, set ninja file name before tool initialization.")

The first of those does intend a warning, it executes code after the call to the class constructor. The second looks like it actually wanted to raise an exception.

Directly raise:

scons/SCons/Tool/ninja/__init__.py

Line 164 in 120b2cd

raise SCons.Warnings.SConsWarning("Failed to import ninja, attempt normal SCons build.")

scons/SCons/Tool/ninja/__init__.py

Line 201 in 120b2cd

raise SCons.Warnings.SConsWarning("Failed to import ninja, attempt normal SCons build.")

These two clearly intend execution to stop.

To fully use the scons framework you define a warning class and assign it to one of the categories (by inheriting from either SConsWarning or WarningOnByDefault) and then invoke it in the desired way by calling SCons.Warnings.warn() (though that's not absolutely necessary). Ninja does define its own warning:

class NinjaExperimentalWarning(SCons.Warnings.WarningOnByDefault)

but it is never used.
@bdbaddog bdbaddog added the Ninja label Nov 1, 2021
@mwichmann
Copy link
Collaborator Author

What should we do here? I'd propose the three in ninja/__init__.py be changed to raise UserError, as they're saying "your setup is wrong, fix it or call SCons a different way". The one in ninja/NinjaState.py could be changed to properly issue a warning, and use the defined ninja warning. I don't see any tests that exercise these cases, so it shouldn't break tests.

@bdbaddog
Copy link
Contributor

bdbaddog commented Oct 5, 2023

That sounds like a reasonable fix to this.

@mwichmann
Copy link
Collaborator Author

Well, it sounded good at the time. More questions now...

In ninja:exists(): if there's no ninja binary, what's the right outcome? And in ninja:generate()? In exists it returns False if 'ninja' not in GetOption('experimental'); is the case of no binary the same? Return False (indicating ninja tool not available) silently? Return False and warn? Raise exception, terminating SCons? Same for generate except no value returned: should we just not initialize the tool? Not initialize, but with warning? Raise exception?

For those two, if the answer is Exception, I think of UserError as error in script, but those are really error in setup (system or virtualenv) - forgot to install module. Is ImportError better? Or some other?

There's a second case in generate, this one describes something you're not allowed to do and seems a clear case of abort with UserError, so I'm not revisiting that one, nor the one that should be a warning in the other file.

mwichmann added a commit to mwichmann/scons that referenced this issue Oct 6, 2023
@mwichmann
Fix ninja warnings/exceptions
1f3ac8b 
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]>
@mwichmann mwichmann linked a pull request Oct 6, 2023 that will close this issue
Draft
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Ninja
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WIP: Fix ninja warnings/exceptions mwichmann/scons
2 participants
@bdbaddog @mwichmann