PR#572: Not create empty skeleton dirs for empty variants - pungi

pungi

#572 Not create empty skeleton dirs for empty variants

Merged 7 years ago by qwan. Opened 7 years ago by qwan.

qwan/pungi not-create-empty-dirs into master

Not create empty skeleton dirs for empty variants

Qixiang Wan • 7 years ago

b880682

pungi/metadata.py

file modified

+13 -1

		`@@ -14,6 +14,7 @@`
		`# along with this program; if not, see <https://gnu.org/licenses/>.`


		`+ import copy`
		`import os`
		`import time`
		`import json`
		`@@ -168,7 +169,18 @@`
		`compose.log_info("[BEGIN] %s" % msg)`

		`path = compose.paths.compose.metadata("composeinfo.json")`
		`- ci.dump(path)`
		`+ # make a copy of composeinfo and modify the copy`
		`+ # if any path in variant paths doesn't exist or just an empty`
		`+ # dir, set it to None, then it won't be dumped.`
		`+ ci_copy = copy.deepcopy(ci)`
		`+ for variant in ci_copy.variants.variants.values():`
		`+ for field in variant.paths._fields:`
		`+ field_paths = getattr(variant.paths, field)`
		`+ for arch, dirpath in field_paths.iteritems():`
		`+ dirpath = os.path.join(compose.paths.compose.topdir(), dirpath)`
		`+ if not (os.path.isdir(dirpath) and os.listdir(dirpath)):`
		`+ field_paths[arch] = None`
		`+ ci_copy.dump(path)`

		`compose.log_info("[DONE ] %s" % msg)`

pungi/phases/createiso.py

file modified

		`@@ -66,6 +66,8 @@`

		`commands = []`
		`for variant in self.compose.get_variants(types=["variant", "layered-product", "optional"]):`
		`+ if variant.is_empty:`
		`+ continue`
		`for arch in variant.arches + ["src"]:`
		`skip_iso = get_arch_variant_data(self.compose.conf, "createiso_skip", arch, variant)`
		`if skip_iso == [True]:`

pungi/phases/extra_files.py

file modified

		`@@ -37,6 +37,8 @@`
		`def run(self):`
		`for arch in self.compose.get_arches() + ["src"]:`
		`for variant in self.compose.get_variants(arch=arch):`
		`+ if variant.is_empty:`
		`+ continue`
		`cfg = get_arch_variant_data(self.compose.conf, self.name, arch, variant)`
		`if cfg:`
		`copy_extra_files(self.compose, cfg, arch, variant, self.pkgset_phase.package_sets)`

pungi/phases/gather/__init__.py

file modified

		`@@ -75,6 +75,8 @@`

		`for arch in self.compose.get_arches():`
		`for variant in self.compose.get_variants(arch=arch):`
		`+ if variant.is_empty:`
		`+ continue`
		`link_files(self.compose, arch, variant,`
		`pkg_map[arch][variant.uid],`
		`self.pkgset_phase.package_sets,`

qwan commented 7 years ago

Do not create empty skeleton dirs for empty variants which we do for rpm
variants in some phases (some others already have the check):
1. createiso phase
2. extra_files phase
3. gather phase

These empty variants will have empty paths in metadata.

FIXES: #497
Signed-off-by: Qixiang Wan qwan@redhat.com

lsedlar commented on line 6 of pungi/metadata.py 7 years ago

This is going to completely exclude the variant from metadata. I don't think that is desirable.

qwan commented on line 6 of pungi/metadata.py 7 years ago

Yes, since the dirs will just be empty, I removed the variants from metadata, one of the exception is iso dir, there may be live isos created under that dir, however it will be listed in images.json so it makes sense to exclude it too. To avoid any risk on this, I'll update it to include these empty variants with empty 'paths'.

rebased

7 years ago

lsedlar commented 7 years ago

Even if the variant is empty, it should still be included in the metadata. If the composeinfo.json is used for example to create a release in PDC, the data should be complete.

Ideally the paths should only be there if the place they point to is not empty.

rebased

7 years ago

ausil commented 7 years ago

composeinfo.json needs to have all the variants listed

qwan commented 7 years ago

Sorry, I updated the patch to include these empty variants with empty paths, but forgot to update the PR description.

rebased

7 years ago

lsedlar commented 7 years ago

I tested this in stage. In the {{composeinfo.json}} all variants are present, but Atomic, CloudImages, Docker, Labs, Spins and ostree variants have no paths.

I'm not sure if that is right though, because for example Labs have some live media in Labs/x86_64/iso/. The same applies for Spins.

CloudImages have content in CloudImages/x86_64/images, but that was not tracked in metadata even before this patch (productmd issue#62).

The directories for Atomic, Docker and ostree do not exist at all. (I think in Docker case it's because the task that should have generated an image failed. Otherwise there would be something in the images/ subdir.)

These are the filesystem changes. I don't see anything obviously wrong with it.

Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/: Atomic
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/CloudImages: i386
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/CloudImages: source
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/CloudImages/x86_64: debug
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/CloudImages/x86_64: iso
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/CloudImages/x86_64: os
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/: Docker
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Labs/i386: debug
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Labs/i386: os
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Labs: source
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Labs/x86_64: debug
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Labs/x86_64: os
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Spins/armhfp: debug
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Spins/armhfp: iso
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Spins/armhfp: os
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Spins/i386: debug
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Spins/i386: os
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Spins: source
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Spins/x86_64: debug
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/Spins/x86_64: os
Only in /mnt/koji/compose/rawhide/Fedora-Rawhide-20170320.n.0/compose/: ostree

qwan commented 7 years ago

I tested this in stage. In the {{composeinfo.json}} all variants are present, but Atomic, CloudImages, Docker, Labs, Spins and ostree variants have no paths.
I'm not sure if that is right though, because for example Labs have some live media in Labs/x86_64/iso/. The same applies for Spins.

Yes, livemedia phase puts the livecd isos under the iso dir, I think these iso paths (created for placing the livecds only) should be tracked in the paths for images, which is (productmd issue#62) you mentioned below as the generated livemedia images are added into compose images.

CloudImages have content in CloudImages/x86_64/images, but that was not tracked in metadata even before this patch (productmd issue#62).
The directories for Atomic, Docker and ostree do not exist at all. (I think in Docker case it's because the task that should have generated an image failed. Otherwise there would be something in the images/ subdir.)
These are the filesystem changes. I don't see anything obviously wrong with it.

lsedlar commented 7 years ago

The images.json file contains all paths correctly. I'm wondering whether it would make sense to add the paths to composeinfo only when the directory actually exists (instead of checking if the variant is empty).

rebased

7 years ago

qwan commented 7 years ago

Patch is updated to check variant paths before dump, if a path doesn't exist or is an empty dir, it will be set to None (in a copied composeinfo), and it will not be dumped.

lsedlar commented 7 years ago

I'll run one more test on stage with the latest patch.

It occurred to me whether PDC will be able to import a compose where some paths are missing. I tested it on current master of PDC and it did not complain and everything looked correct, so it's probably safe.

qwan commented 7 years ago

Commit 258d716 fixes this pull-request

Pull-Request has been merged by qwan@redhat.com

7 years ago