PR#13: Test that the debug build has the proper -O flags set - tests/python

tests / python

#13 Test that the debug build has the proper -O flags set

Merged 4 years ago by churchyard. Opened 4 years ago by churchyard.

tests/ churchyard/python oflags into master

Test that the debug build has the proper -O flags set

Miro Hrončok • 4 years ago

96fbcc5

flags/assertflags.py

file added

+40

		`@@ -0,0 +1,40 @@`
		`+ """`
		`+ This script asserts that Python config vars with compiler options including`
		`+ one or more -O flags have the last specified -O flag equal to the script's`
		`+ first argument.`
		`+`
		`+ We use it to check that the debug build (as well as extension modules) was`
		`+ built with a desired optimization level (usually -Og or -O0).`
		`+`
		`+ About -O flags: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html`
		`+ "If you use multiple -O options, with or without level numbers,`
		`+ the last such option is the one that is effective."`
		`+ """`
		`+`
		`+ import sys`
		`+ import sysconfig`
		`+`
		`+ # The flags that currently don't have the -Og flag on the debug build`
		`+ # and we consider it OK, because we don't know any better :)`
		`+ WHITELIST = [`
		`+ 'CONFIGURE_CFLAGS',`
		`+ 'CONFIGURE_CFLAGS_NODIST',`
		`+ 'CONFIG_ARGS',`
		`+ 'OPT',`
		`+ ]`
		`+`
		`+ print('Expecting that {} is the last -O flag:\n'.format(sys.argv[1]))`
		`+ ret = 0`
		`+`
		`+ for key, flags in sysconfig.get_config_vars().items():`
		`+ if key in WHITELIST:`
		`+ continue`
		`+ if isinstance(flags, str):`
		`+ oflags = [f for f in flags.split(' ') if f.startswith('-O')]`
		`+ if oflags and oflags[-1] != sys.argv[1]:`
		`+ print('Problem in {} -O flags: {}'.format(key, ' '.join(oflags)))`
		`+ ret = 1`
		`+ elif oflags:`
		`+ print('{} are OK'.format(key))`
		`+`
		`+ sys.exit(ret)`

tests.yml

file modified

		`@@ -40,6 +40,9 @@`
		`- debugtest38:`
		`dir: selftest`
		`run: VERSION=3.8 PYTHON="python3-debug" X="test_ssl" ./parallel.sh`
		`+ - debugflags:`
		`+ dir: flags`
		`+ run: python3-debug ./assertflags.py -Og`
		`required_packages:`
		`- gcc`
		`- virtualenv`

churchyard commented 4 years ago

Fixes https://src.fedoraproject.org/tests/python/issue/12

torsava commented on line 21 of flags/assertflags.py 4 years ago

Here I assume you're checking the last element because there might be multiple and you don't care about the non-last ones.

However, are you sure that if there are multiple that the last one will win?

torsava commented on line 22 of flags/assertflags.py 4 years ago

I would add what is the expected flag into the print error output, so it's easier to debug by a new person.

torsava commented 4 years ago

The rest looks good.

churchyard commented on line 21 of flags/assertflags.py 4 years ago

However, are you sure that if there are multiple that the last one will win?

I'm sure that that's the documented behavior. Indeed, there might be a gcc bug, but... testing what actual flag is applied is out of scope here. Let me dig up that doc.

churchyard commented on line 21 of flags/assertflags.py 4 years ago

If you use multiple -O options, with or without level numbers, the last such option is the one that is effective. https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html

1 new commit added

fixup! Test that the debug build has the proper -O flags set

4 years ago

churchyard commented 4 years ago

Pushed a fixup commit for easier review. I plan to squash it once acked.

torsava commented 4 years ago

Ok, sounds good.

torsava commented 4 years ago

The fixup looks good, LGTM!

rebased onto 96fbcc5

4 years ago

churchyard commented 4 years ago

Thanks for the review. Arguably, the script should put errors on stderr, but it's not worth it and currently, the CI splits the stderr and stdout logs, so it would have made it less readable.

churchyard commented 4 years ago

Let me wait on the Fedora Ci output. If it looks good to me, I'll merge here and propose to actually run the test on the python3 package.

Pull-Request has been merged by churchyard

4 years ago

torsava commented 4 years ago

No problem. And the doc on top looks great, good job!