Use more comprehensive auto-formatting tools for Python and C #384

sgallagher · 2019-10-17T03:10:13Z

These patches switch our tests from running autopep8 to running black instead. This autoformatter includes some niceties as enforcing the same style of quotes everywhere and does a better job of aligning on the column boundaries.

They also add support for using clang-tidy instead of clang-format on systems with at least version 9.0.0 of clang-tidy.

I separated the patches that update all of the syntax from the patch that adds the auto-formatters to make review simpler.

I also noticed that .pyc and .pyo files weren't included in .gitignore while working on this, so there's a patch for that as well.

These patches are built atop #383 because that PR includes some fixes that clang-tidy revealed were needed.

On Fedora, Podman is better-supported and has support for CGroupsV2. Fall back to Docker if it is unavailable (such as in the CI environment). Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

mmathesius · 2019-10-17T15:53:44Z

.travis/travis-common.inc

@@ -55,16 +71,16 @@ function mmd_run_docker_tests {
        $SCRIPT_DIR/$MMD_OS/Dockerfile.deps.tmpl > $SCRIPT_DIR/$MMD_OS/Dockerfile.deps.$MMD_RELEASE
    sed -e "s/@RELEASE@/$MMD_OS:$MMD_RELEASE/" $SCRIPT_DIR/$MMD_OS/Dockerfile.tmpl > $SCRIPT_DIR/$MMD_OS/Dockerfile-$MMD_RELEASE

-    sudo docker build \
+    $MMD_BUILDAH \


Based on travis failure, is sudo still needed here and below?

Yeah, sudo is needed for docker but not podman. I'll fix that up. Thanks.

Though that failure is actually different; I was relying on /dev/zero not having the executable flag, but apparently on Ubuntu (where the tests are kicked off), it does... 🤦‍♂️

Nope, that was wrong. I was just missing a variable. I think I will still try to find a less "clever" way of doing this, though.

mmathesius

This looks great to me. Just one small doc fixup request.

Now if we can just figure out what valgrind is complaining about. (I suppose re-ordering the includes could cause something odd to happen.)

mmathesius · 2019-10-17T18:27:44Z

modulemd/include/private/modulemd-subdocument-info-private.h

@@ -109,7 +109,7 @@ modulemd_subdocument_info_get_mdversion (ModulemdSubdocumentInfo *self);
 */
 void
 modulemd_subdocument_info_set_yaml (ModulemdSubdocumentInfo *self,
-                                    const gchar *contents);
+                                    const gchar *yaml);


It looks like my feedback on the first version of this PR got lost...

The naming of the second parameter name should be consistent across the gtk-doc, function declaration, and function implementation. Either change @contents to @yaml in the doc above, or vice versa here and in the .c file.

Yeah, not sure how that happened.

mmathesius · 2019-10-17T18:34:52Z

modulemd/clang_simple_version.sh

@@ -0,0 +1,4 @@
+#!/usr/bin/sh
+
+$1 --version | awk -F 'LLVM version' '{print $2}' - | awk '{print $1}'


Suggested change

$1 --version | awk -F 'LLVM version' '{print $2}' - | awk '{print $1}'

$1 --version | awk '/LLVM version/ {print $NF}' -

Nice trick. I'll use that.

mmathesius · 2019-10-17T18:53:31Z

modulemd/modulemd-module-stream.c

-          updated_stream = modulemd_module_stream_upgrade_to_v2 (
-            current_stream, &nested_error);
+          updated_stream =
+            modulemd_module_stream_upgrade_to_v2 (current_stream);
          if (!updated_stream)
            {
              g_propagate_error (error, g_steal_pointer (&nested_error));


Hmmm. Since nested_error was dropped as a parameter to modulemd_module_stream_upgrade_to_v2(), there isn't (and never was) an nested error to propagate here.

Now if we can just figure out what valgrind is complaining about. (I suppose re-ordering the includes could cause something odd to happen.)

Valgrind is choking on the dup() to fcntl() change. I'm trying to figure out why, but if it takes much longer I may just disable that check.

When run under valgrind, fcntl(fd, F_DUPFD_CLOEXEC) returns -1 and EINVAL. Which according to fcntl(2) means "the kernel does not recognize this value". (Since valgrind overrides fcntl() so it can track resources, it seems like it may be a bug in valgrind itself.)

The issue is that the clang-tidy conversion isn't actually safe. It needs to have a third argument, zero. I've fixed that and it seems to work now.

This syntax guarantees that the file descriptor is closed on exec() Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

https://black.readthedocs.io Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

sgallagher · 2019-10-17T21:01:19Z

I think I've finally worked the bugs out of this PR. Hopefully the CI will pass this time...

sgallagher · 2019-10-17T21:20:23Z

And it looks like CI is passing finally!

mmathesius

Sorry, I should have noticed the missing third argument to fcntl().

All looks good to me now! Thanks for all that clean-up.

sgallagher requested a review from mmathesius October 17, 2019 03:10

sgallagher mentioned this pull request Oct 17, 2019

Use more comprehensive auto-formatting tools for Python and C #380

Closed

sgallagher changed the title ~~Use more comprehensive auto-formatting tools for Python and C~~ [WIP] Use more comprehensive auto-formatting tools for Python and C Oct 17, 2019

sgallagher force-pushed the formatting branch 2 times, most recently from fa05410 to 0baf148 Compare October 17, 2019 15:26

Use podman instead of docker where possible

345208e

On Fedora, Podman is better-supported and has support for CGroupsV2. Fall back to Docker if it is unavailable (such as in the CI environment). Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

sgallagher force-pushed the formatting branch from 0baf148 to bec0c6a Compare October 17, 2019 15:27

mmathesius reviewed Oct 17, 2019

View reviewed changes

sgallagher force-pushed the formatting branch from bec0c6a to a07f74f Compare October 17, 2019 17:31

mmathesius reviewed Oct 17, 2019

View reviewed changes

sgallagher force-pushed the formatting branch from a07f74f to bccd2ff Compare October 17, 2019 20:31

sgallagher added 2 commits October 17, 2019 16:51

Use safer version of dup()

6fc6ff3

This syntax guarantees that the file descriptor is closed on exec() Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

subdocument: match the argument name in the header

13430d9

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

sgallagher force-pushed the formatting branch from bccd2ff to c0ae949 Compare October 17, 2019 20:51

sgallagher added 5 commits October 17, 2019 16:57

Fix invalid nested_error usage

bd65fbf

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Reformat python code with black

b638e0e

https://black.readthedocs.io Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Auto-format python with black

7899340

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Add clang_tidy autoformatter

9306b6d

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Apply clang-tidy formatting

ebaf12b

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

sgallagher force-pushed the formatting branch from c0ae949 to ebaf12b Compare October 17, 2019 20:58

sgallagher changed the title ~~[WIP] Use more comprehensive auto-formatting tools for Python and C~~ Use more comprehensive auto-formatting tools for Python and C Oct 17, 2019

mmathesius approved these changes Oct 17, 2019

View reviewed changes

sgallagher merged commit 028aa0b into fedora-modularity:master Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use more comprehensive auto-formatting tools for Python and C #384

Use more comprehensive auto-formatting tools for Python and C #384

sgallagher commented Oct 17, 2019

mmathesius Oct 17, 2019

sgallagher Oct 17, 2019

sgallagher Oct 17, 2019

sgallagher Oct 17, 2019

mmathesius left a comment

mmathesius Oct 17, 2019

sgallagher Oct 17, 2019

mmathesius Oct 17, 2019

sgallagher Oct 17, 2019

mmathesius Oct 17, 2019

sgallagher Oct 17, 2019

sgallagher Oct 17, 2019

sgallagher commented Oct 17, 2019

sgallagher commented Oct 17, 2019

mmathesius left a comment

		@@ -0,0 +1,4 @@
		#!/usr/bin/sh

		$1 --version \| awk -F 'LLVM version' '{print $2}' - \| awk '{print $1}'

	$1 --version \| awk -F 'LLVM version' '{print $2}' - \| awk '{print $1}'
	$1 --version \| awk '/LLVM version/ {print $NF}' -

Use more comprehensive auto-formatting tools for Python and C #384

Use more comprehensive auto-formatting tools for Python and C #384

Conversation

sgallagher commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmathesius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgallagher commented Oct 17, 2019

sgallagher commented Oct 17, 2019

mmathesius left a comment

Choose a reason for hiding this comment