Skip to content

Conversation

@yangrongwei
Copy link

@yangrongwei yangrongwei commented Sep 21, 2025

Return the relative path rather than the leaf name of the package path.

The leaf name return enforces unnecessary paths to be added to PACKAGES_PATH, solely to satisfy the subsequent call to GetAbsolutePathOnThisSystemFromEdk2RelativePath().

Copy link
Contributor

@Javagedes Javagedes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the behavior of a public function in a widely used distribution is a very significant change. It would introduce major breakages for edk2-pytool-extensions, edk2, project mu, and various other platforms outside of our control.

To do this safely would require a huge amount of effort: updating internal and external consumers, providing a migration path, and adding comprehensive tests. In my view, the workload required is not justified for changing this existing function, especially when there are safer alternatives.

That said, I’m not opposed to the functionality itself — I just think it should be introduced as a new function. For example:

def GetPackagePath(self, package: str) -> str | None:
    """Return the edk2 workspace-relative path to the package's DEC file."""
package = "MyPackage"
# OR
package = edk2path.GetContainingPackage("path/to/my/file.c")

package_path = edk2path.GetPackagePath(package)

This approach gives us the new capability without the large-scale breakage and rework.

@yangrongwei yangrongwei changed the title GetContainingPackage() returns relative path Let GetContainingPackage() return relative path Sep 21, 2025
@Javagedes
Copy link
Contributor

Javagedes commented Sep 21, 2025

I would also be open to moving this code to a new function called GetContainingPackageRelativePath

Then the existing GetContainingPackage simply calls the new GetContainingPackageRelativePath function and returns the folder name.

@yangrongwei
Copy link
Author

Javagedes

Changing the return is safer than you feared.
Adding a new function is messier than you think.

Take the edk2 repo as an example, almost all plug-in call GetAbsolutePathOnThisSystemFromEdk2RelativePath() after GetContainingPackage(). This is a huge effort to change every plug-in.

Current platform owners will not be affected by this change. They provided a superset of PACKAGES_PATH compared to what was expected.

The risk of the order in PACKAGES_PATH is even lower now, because this function returns a more precise relative path.

New platforms will benefit from this change. Since they don’t need to customize PACKAGES_PATH specifically for Stuart, migrating from the build.py based build to Stuart will be easier.

The new GetContainingPackageRelativePath will not provide this benefit unless all plug-ins are migrated to use this new function.

Yours,
Raymond Yang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants