Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wit-parser: Group sources used for resolve by package ids #1868

Conversation

noise64
Copy link
Contributor

@noise64 noise64 commented Oct 16, 2024

This PR is a request for changing the format of the returned source paths for Resolve::push_path and Resolve::push_dir, from Vec<PathBuf> to BTreeMap<PackageId, Vec<PathBuf>>.

Our (Golem Cloud) use case is about using a resolved wit dir and then apply further transformations to it, e.g. managing dependencies between related components. Currently we use a workaround which partially re-parses the wit sources using UnresolvedPackageGroup, and then maps the sources to the resolved package ids. With this change the double parsing could be avoided, as the already available mapping info would not lost, and I believe there can be other tooling related use cases for this information for others too. The original intention for the sources (triggering builds on change) should also work, with the caller side change of collapsing the sources into a unique set.

In case changing this return type is not okay or preferable, I would be also grateful to get pointers on how to approach this problem differently.

Thanks in advance!

@alexcrichton
Copy link
Member

Thanks for the PR! I think this is a reasonable feature to support, and it's ok to change the return types here. I'm wondering though if it might be good to have a dedicated newtype here as the return value with various methods and accessors perhaps. For example one of the main uses I know of this is for bindings generators to generate a list of files to depend on but simple iteration over this map will now produce duplicate listings of files if a WIT package is defined with other inline WIT packages as well.

What do you think about something like:

struct PackageSourceMap { /* ... */ }

impl PackageSourceMap {
    // deduplicates paths so each one only shows up once in the return value
    fn paths(&self) -> impl Iterator<Item = &Path> { /* ... */ }

    // basically BTreeMap::get
    fn package_paths(&self, id: PackageId) -> impl Iterator<Item = &Path> { /* ... */ }
}

Does that seem reasonable?

@noise64
Copy link
Contributor Author

noise64 commented Oct 17, 2024

@alexcrichton Thank you for the response! It it much better that way, I will update the PR soon.

@noise64
Copy link
Contributor Author

noise64 commented Oct 22, 2024

@alexcrichton Updated the PR with the suggested PackageSourceMap.

@alexcrichton alexcrichton added this pull request to the merge queue Oct 22, 2024
Merged via the queue into bytecodealliance:main with commit 28ea1a0 Oct 22, 2024
30 checks passed
@noise64 noise64 deleted the wit-parser-resolve-sources-by-package-id branch October 22, 2024 15:35
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