#521 deps: use mock shell instead of `--pm-cmd`
Merged 4 months ago by ngompa. Opened 4 months ago by sbonazzo.
sbonazzo/FedoraReview master  into  master

file modified
+13 -13
@@ -91,11 +91,11 @@ 

          *shlex.split(Settings.mock_options),

          "-qn",

          "--enable-network",

-         "--pm-cmd",

+         "--shell",

          "--",

-         "repoquery", "-q", "-C", "--requires", "--resolve",

+         "dnf", "repoquery", "-q", "-C", "--requires", "--providers-of=requires",

      ]

-     cmd += list(set(pkgs))

+     cmd += [f"'{x}'" for x in set(pkgs)]

  

      Settings.get_logger().debug("Running: %s", " ".join(cmd))

      try:
@@ -128,11 +128,11 @@ 

          *shlex.split(Settings.mock_options),

          "-qn",

          "--enable-network",

-         "--pm-cmd",

+         "--shell",

          "--",

-         "repoquery", "-q", "-C", "--provides",

+         "dnf", "repoquery", "-q", "-C", "--provides",

      ]

-     cmd += list(set(pkgs))

+     cmd += [f"'{x}'" for x in set(pkgs)]

  

      Settings.get_logger().debug("Running: %s", " ".join(cmd))

      try:
@@ -174,9 +174,9 @@ 

          *shlex.split(Settings.mock_options),

          "-qn",

          "--enable-network",

-         "--pm-cmd",

+         "--shell",

          "--",

-         "repoquery", "-q", "-C", "--whatprovides", req,

+         "dnf", "repoquery", "-q", "-C", "--whatprovides", f"'{req}'",

      ]

      Settings.get_logger().debug("Running: %s", " ".join(cmd))

  
@@ -273,9 +273,9 @@ 

              *shlex.split(Settings.mock_options),

              "-qn",

              "--enable-network",

-             "--pm-cmd",

+             "--shell",

              "--",

-             "repoquery", "-C", "--quiet", "--file", path,

+             "dnf", "repoquery", "-C", "--quiet", "--file", path,

          ]

          Settings.get_logger().debug("Running: %s", " ".join(cmd))

          try:
@@ -306,11 +306,11 @@ 

          *shlex.split(Settings.mock_options),

          "-qn",

          "--enable-network",

-         "--pm-cmd",

+         "--shell",

          "--",

-         "repoquery", "-C", "-l",

+         "dnf", "repoquery", "-C", "-l",

      ]

-     cmd += list(set(pkgs))

+     cmd += [f"'{x}'" for x in set(pkgs)]

  

      Settings.get_logger().debug("Running: %s", " ".join(cmd))

      try:

Running mock with -qn and --pm-cmd shows no output.
In order to get meaningfull output with -qn we need to use
--shell and call dnf directly.

Addressed also a missing migration from dnf to dnf5
as it was hidden by the execution through --pm-cmd: changed
--resolve in --providers-of=requires.

Having empty output caused several issues including a wrong detection of
directories ownership as reported and discussed in issue #515.

Fixes: #515

Signed-off-by: Sandro Bonazzola sbonazzo@redhat.com

Pull-Request has been merged by ngompa

4 months ago

I introduced the --pm-cmd command recently to support both DNF and DNF5
https://pagure.io/FedoraReview/pull-request/513

We should make sure this PR doesn't break for either of them.

I introduced the --pm-cmd command recently to support both DNF and DNF5
https://pagure.io/FedoraReview/pull-request/513

We should make sure this PR doesn't break for either of them.

well, with --pm-cmd both of them were broken as it didn't produce any output... But compared to previous version here we are not calling dnf-3 but dnfso it should work fine both on dnf-3 based and on dnf-5 based distro. The only point which may not work well on dnf-3 based distro is repolist as dnf-3 requires --resolve while dnf-5 requires --providers-of=requires. But honestly, I think it doesn't make sense using fedora-review on something different than Rawhide.

I finally remembered why the --pm-cmd was needed and why I implemented PR #513.
Without it, fedora-review doesn't work when Mock tmpfs plugin is enabled.

Please try putting this into your /etc/mock/site-defaults.cfg:

config_opts['plugin_conf']['tmpfs_enable'] = True
config_opts['plugin_conf']['tmpfs_opts'] = {}
config_opts['plugin_conf']['tmpfs_opts']['required_ram_mb'] = 1024
config_opts['plugin_conf']['tmpfs_opts']['max_fs_size'] = '140g'
config_opts['plugin_conf']['tmpfs_opts']['mode'] = '0755'
config_opts['plugin_conf']['tmpfs_opts']['keep_mounted'] = False

This is an important use-case because Copr builders use this configuration.

I may be wrong but I think tmpfs can't be used with Fedora review because the content of mock buildroot is not persisted. Using --pm-cmd just hide the failures because it doesn't output anything.
It runs all of the commands with --no-cleanup-after --no-clean and it assumes the rootdir is preserved from one command to the next one while using tmpfs re-creates the root dir every time on a clean tmpfs filesystem. So either the usage of tmpfs should be disabled when running fedora-review on copr systems or the whole fedora-review tool need to be re-designed assuming root doesn't persist.

What about adding --disable-plugin=tmpfs to https://pagure.io/FedoraReview/blob/master/f/src/FedoraReview/settings.py#_77 ?

Good idea. I will try if it works and get back to you :-)

Also, I think it may be enough to keep the tmpfs plugin enabled but only setting

config_opts['plugin_conf']['tmpfs_opts']['keep_mounted'] = True

via a command line parameter.

@sbonazzo I think it works. Can you please try PR #522 to see if it works for you too?

@frostyx works for me too, commented there.

Metadata