Skip to content

bring back identity improvements #3344

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 25, 2021
Merged

bring back identity improvements #3344

merged 1 commit into from
Mar 25, 2021

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Mar 10, 2021 •
edited
Loading

Bring back #3323

motivation: improve correctness by using identity more broadly throughout the codebase

changes:

  • expose identity on Package, ResolvedPackage and GraphLoadingNode
  • reduce calls to identityResolver.resolveIdentity since we now have identity more broadly available (also improves performance)
  • ManifestLoader now take PackageIdentity to reduce calls to identityResolver.resolveIdentity when the identity is already known
  • adjust call-sites
  • adjust tests

tomerd
tomerd commented Mar 10, 2021
View reviewed changes
@tomerd tomerd self-assigned this Mar 10, 2021
@tomerd
Copy link
Contributor Author

tomerd commented Mar 10, 2021

@swift-ci please test

@tomerd
Copy link
Contributor Author

tomerd commented Mar 10, 2021

@swift-ci please self-hosted test

@tomerd
Copy link
Contributor Author

tomerd commented Mar 10, 2021

@swift-ci please test self-hosted

@tomerd
Copy link
Contributor Author

tomerd commented Mar 10, 2021

@swift-ci please smoke test self hosted

@tomerd
Copy link
Contributor Author

tomerd commented Mar 10, 2021

@swift-ci please test package compatibility

abertelrud
abertelrud approved these changes Mar 11, 2021
View reviewed changes
@abertelrud
Copy link
Contributor

Agreed, this API should be reworked. There is definitely a balance here between avoiding cascading errors vs returning as much information as make sense for IDEs (or other tools) to show, even in the presence of some errors.

Hmm, that was supposed to be a response to a particular FIXME comment in the code about needing to rework this to be more resilient to errors. Don't know why the comment became dissociated from the code.

@tomerd
Copy link
Contributor Author

tomerd commented Mar 11, 2021

@swift-ci please smoke test

@abertelrud
Copy link
Contributor

Linux failure looks quite unrelated indeed:

14:19:40 
bindings/python/LLDBWrapPython.cpp:80128:1: error: 'LLDBSwigPythonBreakpointCallbackFunction' has C-linkage specified, but returns user-defined type 'llvm::Expected<bool>' which is incompatible with C [-Werror,-Wreturn-type-c-linkage]
14:19:40 LLDBSwigPythonBreakpointCallbackFunction
14:19:40 ^
14:19:40 1 error generated.

@tomerd
Copy link
Contributor Author

tomerd commented Mar 11, 2021 •
edited
Loading

Linux failure looks quite unrelated indeed:

yea this is a know issue with the toolchain rn. will trigger those test again later

@abertelrud
Copy link
Contributor

Linux failure looks quite unrelated indeed:

yea this is a know issue with the toolchain rn. will trigger those test again later

I see — hadn't seen this one before myself so figured I would highlight it in case it saved some time.

@tomerd
Copy link
Contributor Author

tomerd commented Mar 11, 2021

@swift-ci test

@tomerd
Copy link
Contributor Author

tomerd commented Mar 11, 2021

@swift-ci test linux

@neonichu
Copy link
Contributor

introduce PackageIdentity.root as a utility for root packages, since their identity is straight-forward

Does that mean root packages have an identity distinct from dependencies? That doesn't sound correct to me since root packages can override dependencies with the same identity today.

@tomerd
Copy link
Contributor Author

tomerd commented Mar 11, 2021 •
edited
Loading

Does that mean root packages have an identity distinct from dependencies? That doesn't sound correct to me since root packages can override dependencies with the same identity today.

this is a good point. I ended up not using PackageIdentity.root for this reason, so maybe I should just call it something different or even remove it.

I do think we need to overhaul the "override" behavior to be explicit, but that is a topic for another PR

@tomerd tomerd force-pushed the revert-3342-revert-3323-refactor/identity-04 branch from 4db506a to 650c4ba Compare March 11, 2021 21:44
@tomerd
Copy link
Contributor Author

tomerd commented Mar 11, 2021

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Mar 11, 2021

@swift-ci please test package compatibility

@neonichu
Copy link
Contributor

I do think we need to overhaul the "override" behavior to be explicit, but that is a topic for another PR

Absolutely. IIRC we have been discussing some solutions as part of the registry conversations, right?

@tomerd tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Mar 12, 2021
@tomerd
identity improvements
14fd2a5 
motivation: improve correctness by using identity more broadly throughout the codebase

changes:
* expose identity on Package, ResolvedPackage and GraphLoadingNode
* reduce calls to identityResolver.resolveIdentity since we now have identity more broadly available (also improves performance)
* ManifestLoader now take PackageIdentity to reduce calls to identityResolver.resolveIdentity when the identity is already known
* adjust call-sites
* adjust tests
@tomerd tomerd force-pushed the revert-3342-revert-3323-refactor/identity-04 branch from 650c4ba to 14fd2a5 Compare March 23, 2021 21:37
@tomerd
Copy link
Contributor Author

tomerd commented Mar 23, 2021

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Mar 23, 2021

unrelated linux ci issue

@swift-ci please smoke test linux

@tomerd
Copy link
Contributor Author

tomerd commented Mar 23, 2021

@swift-ci please test package compatibility

@tomerd tomerd merged commit 06f9b30 into main Mar 25, 2021
@tomerd tomerd deleted the revert-3342-revert-3323-refactor/identity-04 branch July 29, 2021 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abertelrud abertelrud abertelrud approved these changes

@friedbunny friedbunny Awaiting requested review from friedbunny

@neonichu neonichu Awaiting requested review from neonichu

Assignees

@tomerd tomerd

Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomerd @abertelrud @neonichu