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

Add ability for SConscript to fail on missing script #3154

Merged
merged 7 commits into from
Aug 7, 2018

Conversation

mwichmann
Copy link
Collaborator

@mwichmann mwichmann commented Jul 21, 2018

SConscript call now takes an optional must_exist flag, which defaults
to False for compatiility with current behavior. If True, an exception
is raised if the file is missing. To improve readability, the
decision is moved off to a new function rather than being inline in
_SConscript.

A global setting to control the overall behavior is also added.

A deprecation warning is added for the current behavior, which is
printed only once.

A test case is now available.

Fixes #3162

Contributor Checklist:

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

SConscript call now takes an optional must_exist flag, which defaults
to False for compatiility with current behavior.  If True, an exception
is raised if the file is missing.  To improve readability, the
decision is moved off to a new function rather than being inline in
_SConscript.

A global setting to control the overall behavior is also added.

A deprecation warning is added for the current behavior, which is
printed only once.

Signed-off-by: Mats Wichmann <mats@linux.com>
@bdbaddog
Copy link
Contributor

If you can add some tests for this change I think we'll be able to merge it.
Also add must_exist to the docstring per my other comment

Testcases added to confirm the behavior of: first attempt
to call a non-existent script gives a deprecation warning,
additional ones give plain warning; True/False values for
must_warn behave as expected; if scons default is changed
to exception the call fails but if must_warn=False it
still works.

Tweaked the logic to actually get that last bit to work.
Also minor doc update.

Signed-off-by: Mats Wichmann <mats@linux.com>
@mwichmann
Copy link
Collaborator Author

mwichmann commented Jul 28, 2018

Re: adding must_exist to the docstring... there doesn't seem to be a logical place to do this. SConscript.py/_SConscript() doesn't have a docstring now, and SConscript.py/SConscript() has one which is not related to arguments, rather describes how strack trace prints will be handled. I must be missing the point.

@bdbaddog
Copy link
Contributor

Every time I update a function or a method, I've been adding proper docstrings (describe arguments and return values). So that's what I'm requesting you do if possible.

The idea is to make the API docs more useful over time.

@mwichmann
Copy link
Collaborator Author

I understand now. I'll see about this... SConscript is fairly rich interface so it's not trivial. Can scons help with an arg spit out the docsrting for that method/builder?

@bdbaddog
Copy link
Contributor

You might try using file fixture for your test's SConstruct
(https://github.com/SCons/scons/wiki/TestingMethodology search for "File fixtures")

Signed-off-by: Mats Wichmann <mats@linux.com>
@mwichmann
Copy link
Collaborator Author

mwichmann commented Jul 29, 2018

Made an effort at a docstring. This is in the google style, which as I recall doesn't fully work with the current obsolete doc builder (see #3121 for that issue). I can recast it in reST style if preferred, although I never seem to get those to work right.

@mwichmann
Copy link
Collaborator Author

I could redo this as a file fixture, but it's not really that complicated. New to these tests, not sure if it's better to try to write it as multiple distinct test cases, or an all-in-one like this effort did.

The test is running and passing, even though the Travis CI builds are not overall passing, did not examine those yet.

Py2.7:

    736/1175 (62.64%) /home/travis/virtualenv/python2.7.14/bin/python -tt test/SConscript/must_exist.py
    PASSED

Py3.6:

    736/1175 (62.64%) /home/travis/virtualenv/python2.7.14/bin/python -tt test/SConscript/must_exist.py
    PASSED

@bdbaddog
Copy link
Contributor

The test is fine as is.
I guess the only thought is if one of the cases starts to fail, how easy is it to debug.
(This is really a nitpick, but I've been fighting with debugging failing tests lately and a lot when I worked on the py3 port)

The Docstring is great! Thanks for all the effort here.
I'm going to merge.
I may add a bigger blurb on the must_exist in the src/CHANGES.txt prior to release to call attention to it.

@bdbaddog
Copy link
Contributor

oh. Hold up. Your new test and one other are failing on travis ci.
Can you review and resolve?

@mwichmann
Copy link
Collaborator Author

mwichmann commented Jul 29, 2018

okay, two tests expected the existing warning string, and are unhappy that the first-time message is the deprecation warning with different text. I can update those two to expect the new message - is that ok? Can also get rid of the deprecation message...

@bdbaddog
Copy link
Contributor

bdbaddog commented Jul 29, 2018

Sure.

@mwichmann
Copy link
Collaborator Author

There was actually a subtle problem as well as wrong string... the addition/use of the DeprecatedMissingSConscriptWarning class broke the command-line option to turn off warnings on missing sconscripts.

Signed-off-by: Mats Wichmann <mats@linux.com>
@coveralls
Copy link

coveralls commented Jul 30, 2018

Coverage Status

Coverage increased (+0.8%) to 80.663% when pulling b4d4d28 on mwichmann:wip-missing-scons into ee03bf1 on SCons:master.

mwichmann and others added 3 commits July 30, 2018 07:33
Also handle_missing_SConscript(), internal interface added by
this patch series.

Signed-off-by: Mats Wichmann <mats@linux.com>
@bdbaddog
Copy link
Contributor

bdbaddog commented Aug 7, 2018

Minor edit to resolve conflict on CHANGES.txt.

Merging! Thanks for the good work!

@bdbaddog
Copy link
Contributor

bdbaddog commented Aug 7, 2018

oh. I'll wait for regressions to complete and the merge.

@bdbaddog bdbaddog merged commit 9e661c6 into SCons:master Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants