Buildsystem optimization

[j-ode]

Description:

• Speed up the build system by using C implementations of yaml Loader and Dumper in ssg/yaml.py used by build-scripts/collect_remediations.py
• Fix build profiler bug where targets with multiple outputs all added duplicate build time
• What is the difference in build time after these changes (RHEL8 build):
  • before: ~3 minutes, collect_remediations taking up ~17% of build time and ~30 seconds itself
  • after: ~2:45 minutes, collect_remediations taking up ~12% of build time and ~20 seconds itself
  • these times were measured using the profiling tool from Implemented buildsystem profiling #7832, they vary a lot and are averages from multiple builds

Issues:

• When using CDumper, the formatting fix on lines 150-151 in yaml.py/ordered_dump does not work. This is seemingly because the underlying emitter classes are different for yaml.Dumper and yaml.CDumper, and while the former contains the increase_indent method, the latter does not (https://github.com/yaml/pyyaml/blob/8cdff2c80573b8be8e8ad28929264a913a63aa33/yaml/_yaml.pyx and https://github.com/yaml/pyyaml/blob/8cdff2c80573b8be8e8ad28929264a913a63aa33/lib/yaml/emitter.py, respectively). I found that some people had the exact same issue here: Indentation of sequence elements yaml/libyaml#224 and there was no response from the maintainers of libyaml. Although yamllint says that some ansible tasks in build/rhel8/fixes/ansible have invalid indentation, running the test suite on some of these ansible tasks (such as sssd_offline_cred_expiration) with the --remediate-using ansible option produced no errors, so I am not sure if this is actually a problem. If needed, a simple regex substitution could be added to the ordered_dump function to add the extra indentation (modifying and recompiling libyaml seems needlessly complicated).

[pep8speaks]

Hello @j-ode! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-06-15 15:06:48 UTC

[github-actions]

Start a new ephemeral environment with changes proposed in this pull request:

[jan-cerny]

• it seems to be a great idea!
• in theory the 2 implementation should be equivalent
• the changes in CMake should definitely be a separate commit, preferably they would be a separate pull request
• we have some tests failing in the gating runs so please investigate them

[jan-cerny]

Unfortunately, I have found that there are situations in which this causes incorrect form of data in the built data stream. For example, in ssg-rhel8-ds.xml the Ansible remediation for rule accounts_password_pam_minlen contains create: 'yes'. But it should contain create: yes, because in Ansible lineinfile module the create key must be a boolean, the yes without quotes is a boolean but 'yes' in quotes is a string. Fortunately, Ansible understands it because it attempts to perform an implicit typecast and if you pass 'yes' Ansible converts it to yes internally. So it probably doesn't break the content. But I don't know if there are rules in which this subtle change will matter and I also better wouldn't rely on this undocumented typecast behavior of Ansible. We will need to find out why the quotes around yes got added.

[jan-cerny]

So there is the "suspicious" line 33 which modifies yaml_SafeLoader:

yaml_SafeLoader.add_constructor(u'tag:yaml.org,2002:bool', _bool_constructor)

But before your patch the yaml_SafeLoader was used only in the _open_yaml function. So this special modfication from line 33 affected only the _open_yaml function. But, you start to use the yaml_SafeLoader also here in ordered_load, so now this special modification affects also the ordered_load function.

If we would like to use yaml_SafeLoader in both functions but keep the previous behavior of ordered_load, we would have to rework it so that the special modification from line 33 would again affect only _open_yaml. We can do that using a trick like this:

diff --git a/ssg/yaml.py b/ssg/yaml.py
index 71f7e3adee..601584a21d 100644
--- a/ssg/yaml.py
+++ b/ssg/yaml.py
@@ -29,8 +29,6 @@ def _unicode_constructor(self, node):
     return str(string_like)
 
 
-# Don't follow python bool case
-yaml_SafeLoader.add_constructor(u'tag:yaml.org,2002:bool', _bool_constructor)
 # Python2-relevant - become able to resolve "unicode strings"
 yaml_SafeLoader.add_constructor(u'tag:yaml.org,2002:python/unicode', _unicode_constructor)
 
@@ -52,8 +50,13 @@ def _open_yaml(stream, original_file=None, substitutions_dict={}):
 
     Return None if it contains "documentation_complete" key set to "false".
     """
+    class OurCustomLoader(yaml_SafeLoader):
+        pass
+
+    # Don't follow python bool case
+    OurCustomLoader.add_constructor(u'tag:yaml.org,2002:bool', _bool_constructor)
     try:
-        yaml_contents = yaml.load(stream, Loader=yaml_SafeLoader)
+        yaml_contents = yaml.load(stream, Loader=OurCustomLoader)
 
         if yaml_contents.pop("documentation_complete", "true") == "false" and \
                 substitutions_dict.get("cmake_build_type") != "Debug":

I admit that I don't like that we have some special things like this yaml_SafeLoader.add_constructor(u'tag:yaml.org,2002:bool', _bool_constructor), it would be ideal to remove it, but that would be very very difficult because a lot of code depend on this.

[j-ode]

Thank you for noticing this. From my testing, it seems this conversion was caused by using CSafeLoader, when using CLoader the issue is gone.

[jan-cerny]

should contain create: yes

or it should contain create: true which is the case that was there before this PR

[codeclimate]

Code Climate has analyzed commit 74d2270 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	1
Complexity	1

The test coverage on the diff in this pull request is 63.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 42.8% (0.0% change).

View more on Code Climate.

[jan-cerny]

I experience a smaller speedup, on my machine ~ 6 seconds, but that's a great achievement! Kudos