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

Clean up CC and CXX FLAGS tests #4693

Merged
merged 4 commits into from
Apr 6, 2025
Merged

Conversation

mwichmann
Copy link
Collaborator

More consistency, formatting, add test/CXX/SHCCFLAGS.py for symmetry (test/CC/ had one), rename tests to -live since they use a real compiler, and also add that note to docstring. Remove some filename suffix fiddling that wasn't needed because in this usage, SCons figures it out - and we want that as part of the test.

Testsuite-only change.

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 RELEASE.txt (and read the README.rst).
  • I have updated the appropriate documentation

@mwichmann mwichmann added the testsuite Things that only affect the SCons testing. Do not use just because a PR has tests. label Mar 7, 2025
@mwichmann
Copy link
Collaborator Author

using an underscore is basically saying "I know this returns something, and I actually mean to ignore it".

@bdbaddog
Copy link
Contributor

bdbaddog commented Mar 8, 2025

using an underscore is basically saying "I know this returns something, and I actually mean to ignore it".

Yup. I understand that, but I'd rather not sugguest to the community that using the return value for DefaultEnvironment() is a good idea.

@mwichmann
Copy link
Collaborator Author

Then the public interface probably ought not to return anything... yeah, I can get rid of this usage if you're insistent, but it seems worthwhile to quiet IDEs. I guess the use case for it returning an env is so that you can examine it - what compilers did it find, etc.. Should we say in docs that you should not call methods on the returned env? There are some funky internal uses like:

json_node = SCons.Defaults.DefaultEnvironment().arg2nodes(json)

path = SCons.Defaults.DefaultEnvironment()._CacheDir_path

@bdbaddog
Copy link
Contributor

bdbaddog commented Mar 8, 2025

Then the public interface probably ought not to return anything... yeah, I can get rid of this usage if you're insistent, but it seems worthwhile to quiet IDEs. I guess the use case for it returning an env is so that you can examine it - what compilers did it find, etc.. Should we say in docs that you should not call methods on the returned env? There are some funky internal uses like:

json_node = SCons.Defaults.DefaultEnvironment().arg2nodes(json)

path = SCons.Defaults.DefaultEnvironment()._CacheDir_path

There's lots of use in the wild, so changing the return value would break a lot.
It will work, it's just not good practice, so omitting such from our own usage is probably sufficient?

@mwichmann
Copy link
Collaborator Author

There's lots of use in the wild, so changing the return value would break a lot. It will work, it's just not good practice, so omitting such from our own usage is probably sufficient?

$ git grep "=.*DefaultEnvironment" \*.py | wc
     66     226    4598

that's on master, thus omitting the changes from the PR, so there are a bunch of them (three of those are false hits, they're actually DefaultEnvironmentCall, so it's 63 by that unsophisticated grep).

@mwichmann
Copy link
Collaborator Author

Rolled back the assignment, now it's just a bare call.

More consistency, formatting, add test/CXX/SHCCFLAGS.py for symmetry
(test/CC/ had one), rename tests to -live since they use a real compiler,
and also add that note to docstring. Remove some filename suffix
fiddling that wasn't needed because in this usage, SCons figures it out -
and we wnat that as part of the test.

Signed-off-by: Mats Wichmann <[email protected]>
No longer assign the result of calling DefaultEnvironment to a dummy.

Signed-off-by: Mats Wichmann <[email protected]>
@bdbaddog bdbaddog merged commit 038dc4b into SCons:master Apr 6, 2025
6 of 8 checks passed
@mwichmann mwichmann moved this to Done in Next Release Apr 6, 2025
@mwichmann mwichmann added this to the NextRelease milestone Apr 6, 2025
@mwichmann mwichmann deleted the test/flagstests branch April 6, 2025 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsuite Things that only affect the SCons testing. Do not use just because a PR has tests.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants