Skip to content

[wip] unwind target based dependencies #3829

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

Closed

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Nov 1, 2021

motivation: target based dependencies implementation of unused products never materialized. we need to revisit this topic, but until then we would like to simplify the codebase by removing the partial implementation

changes:

motivation: target based dependencies implementation of unused products never materialized. we need to revisit this topic, but until then we would like to simplify the codebase by removing the partial implementation

changes:
* undo changes from Finish SE‐0226 (Ignore Unused Products) swiftlang#2749
* adjust call-sites and test which were added or modified after Finish SE‐0226 (Ignore Unused Products) swiftlang#2749 was merged
@tomerd tomerd force-pushed the refactor/unwind-target-based-deps branch from a4ffb66 to d19f9b2 Compare November 1, 2021 20:54
@tomerd
Copy link
Contributor Author

tomerd commented Nov 1, 2021

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Nov 1, 2021

@swift-ci please test package compatibility

@tomerd
Copy link
Contributor Author

tomerd commented Nov 2, 2021

@abertelrud @neonichu with package compatibility test suite passing, do we feel the approach here is good and we should take this change? if so, I can clean up the PR and get it ready for merging

@SDGGiesbrecht
Copy link
Contributor

Has anyone tested it again since identities were overhauled?

I repeat that I would be happy to do the debugging if you can provide a concrete failing package.

@tomerd
Copy link
Contributor Author

tomerd commented Nov 3, 2021

@SDGGiesbrech before opening this PR I spent some time trying to get the implementation to work by removing the ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION gate but could not get the tests gated by this flag to pass. maybe too much has changed in main?

an alternative PR that restores this functionality could be a good comparison. my goal win this one was to "reset" the state before taking another stab at this, or iow to reduce complexity / remove unused code before we attempt this again

wdyt?

@SDGGiesbrecht
Copy link
Contributor

If main has changed that much, then go ahead with this PR if it simplifies other work.

In the next few days I will try reverting to debug the failing tests, but I don’t need it to be present in main to do that. If I can figure out what is wrong and get them passing again, then I will send a new PR, and you can use that to try it on whatever your top secret packages are.

Feel free to try to beat me to it and implement it yourself. I just want this feature to finally be available.

@SDGGiesbrecht SDGGiesbrecht mentioned this pull request Nov 5, 2021
11 tasks
@neonichu neonichu closed this Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants