rez.package_maker.make_package is bad

[vanridal]

We had a issue where a bad pip package got created due to a network issue and we discovered it is down to how rez.package_maker.make_package makes a package.

rez/src/rez/package_maker.py

Line 225 in 9c1cdee

variant_ = variant.install(path)

for variant in src_variants:
    variant_ = variant.install(path)

    base = variant_.base
    if make_base and base:
        os.makedirs(base, exist_ok=True)
        os.chdir(base)
        make_base(variant_, base)

    root = variant_.root
    if make_root and root:
        os.makedirs(root, exist_ok=True)
        os.chdir(root)
        make_root(variant_, root)

    maker.installed_variants.append(variant_)

variant_ = variant.install(path)
Creates a package.py and the package is then available to resolves before any data has been written to the payload. It also does not create .building{version} tag in the repo to make it invisible to resolves till building is done.

I suggest changing this to be more in line with how a build process builds a package by calling repository.pre_variant_install(variant.resource) and only variant.install once the package has data in the payload

Also a build process uses repository.get_package_payload_path() to get the variant.base

Environment

• Rez version ("3.4.0")
• Rez python version ("3.11")

[JeanChristopheMorinPerso]

I looked at this the other day and I don't know if the solution is as simple as what you propose. The make_package context manager function is weird because it's not using a package definition. The variant instance available in that function is a memory-backed variant resource object (from the memory package repository plugin):

rez/src/rez/package_maker.py

Lines 135 to 146 in 4946f9d

	repo = create_memory_package_repository(repo_data)
	# retrieve the package from the new repository
	family_resource = repo.get_package_family(self.name)
	it = repo.iter_packages(family_resource)
	package_resource = next(it)
	package = self.package_cls(package_resource)
	# revalidate the package for extra measure
	package.validate_data()
	return package

The callbacks receive two arguments (the variant object and a path).

Here are some thoughts: https://ampcode.com/threads/T-019f2d8b-6ab5-77bd-8a05-40c81442292f. Obviously, this is an LLM, but I think it's getting on the right track in some ways.

I never really liked the shape of make_package, but it's a widely used function and we can't break the API contract. Maybe we can create a new function that is nicer and easier to use?

[vanridal]

Ive been testing this code below, and works as expected for both rez-pip and rez-pip2 (0.4.2).
The change does effect the resource attached to the the variant object passed to make_base() and make_root(), as you said it will be a Memory variant resource.

To move this forward, I can create a PR for this plus add some testing to validate expected behavior, as the LLM review suggested.

The code path mirrors what occurs in a local build_process.

rez/src/rezplugins/build_process/local.py

Line 158 in 4946f9d

package_install_path = self.get_package_install_path(install_path)

    with retain_cwd():
        # install the package variant(s) into the filesystem package repo at `path`
        for variant in src_variants:
            repository = package_repository_manager.get_repository(path)

            base = repository.get_package_payload_path(
                    package_name=variant.name,
                    package_version=variant.version
            )
            repository.pre_variant_install(variant.resource)

            if make_base and base:
                if not os.path.exists(base):
                    os.makedirs(base)
                os.chdir(base)
                make_base(variant, base)

            if variant.index is None:
                root = base
            else:
                root = os.path.join(base, variant._non_shortlinked_subpath)

            if make_root and root:
                if not os.path.exists(root):
                    os.makedirs(root)
                os.chdir(root)
                make_root(variant, root)

            variant_ = variant.install(path)
            maker.installed_variants.append(variant_)

(posted from the correct account this time)

[vanridal]

@JeanChristopheMorinPerso here is a PR with the changes I proposed above.
#2177

If there is concerns on how this changes the api for consumers perhaps maybe better saved for a major release?