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

WIP: Fix ninja warnings/exceptions #4427

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mwichmann
Copy link
Collaborator

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.

There are no API or documentation impacts of this change, just consistency.

Fixes #4036

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

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 <mats@linux.com>
@mwichmann mwichmann added the maintenance Tasks to maintain internal SCons code/tools label Oct 6, 2023
@mwichmann
Copy link
Collaborator Author

mwichmann commented Oct 6, 2023

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).

	test/ninja/generated_sources_alias.py
	test/ninja/ninja_command_line.py

@mwichmann
Copy link
Collaborator Author

That's more interesting than I expected - looks like this is an internal action - it comes from the Textfile builder, and the comments in SCons/Tool/ninja/NinjaState.py make me think a different action name was expected. Here's one of the new warnings:

> scons: warning: Found unhandled function action '_action',  generating SCons command to build
> Note: this is less efficient than using Ninja. You can write your own ninja build generator for this function using NinjaRegisterFunctionHandler
> File "/home/mats/github/scons/scripts/scons.py", line 97, in <module>

@mwichmann mwichmann changed the title Fix ninja warnings/exceptions WIP: Fix ninja warnings/exceptions Oct 6, 2023
@mwichmann
Copy link
Collaborator Author

Flagging this WIP/draft until some questions are answered.

@mwichmann mwichmann marked this pull request as draft October 6, 2023 16:35
Signed-off-by: Mats Wichmann <mats@linux.com>
@bdbaddog
Copy link
Contributor

bdbaddog commented May 19, 2024

@mwichmann Is this still viable, or should it be closed?

@mwichmann
Copy link
Collaborator Author

@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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Tasks to maintain internal SCons code/tools Ninja
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ninja tool: using SCons warnings framework incorrectly
2 participants