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

Sort libraries first by dependency, then apply user set dependency order #169

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kudaba
Copy link
Contributor

@kudaba kudaba commented Feb 8, 2022

Updated version of #86 which got closed when I renamed the branch. This new version is updated to also sort dependent library includes to the whole graph is in order.

The alternative to this would be just dropping /Wl,start-group/end-group around all libraries which works but might add link cost since it iterates over the whole set over and over until all symbols are satisfied (success) or no more symbols can be found (error).

@@ -254,6 +254,13 @@ public uint DependenciesOrder
set { SetProperty(ref _dependenciesOrder, value); }
}

private bool _autoDependenciesOrder = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this shouldn't rather be on the conf itself, so it would allow conditionally activating it on platforms where it matter, like linux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could go either way, I suppose I just figured it doesn't hurt on non-linux platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I'd put it in the conf, it allows greater flexibility

@belkiss
Copy link
Contributor

belkiss commented Feb 10, 2022

Would be great if you could add unit-tests for this :)

@kudaba kudaba force-pushed the pr/auto_dependency_ordering branch from ef7c6bf to 1794f9a Compare February 11, 2022 06:05
@kudaba
Copy link
Contributor Author

kudaba commented Feb 11, 2022

Unit tests did expose a weakness in how I generate the order, so that's good, but it also shows that I'm overriding an order value of shared objects which means if I don't want it to effectively be a global setting then I'd need to have some way to generate the auto order during link and reset it after. I'll have to tinker more with it.

The start/end group option is looking pretty nice right about now. I really have no idea what the perf cost of doing it that was is though, I'm more trying to do the auto ordering to keep with the principle of only cost what's absolutely necessary.

…rder, then apply user order to resolve any conflicts at the same dependency level.
@kudaba kudaba force-pushed the pr/auto_dependency_ordering branch from 1794f9a to 2f407eb Compare February 20, 2022 06:48
@kudaba kudaba changed the title Add an AutoDependenciesOrder option Sort libraries first by dependency, then apply user set dependency order Feb 20, 2022
@kudaba
Copy link
Contributor Author

kudaba commented Feb 20, 2022

After some testing I realized there wasn't a good way to make this an option, unless it's a global toggle which would be easy to add if you want. I don't think there should be any harm in applying this all the time though since it should be a net benefit unless people were not declaring dependencies and relying on target order to fix linux builds anyway.

@jspelletier jspelletier changed the base branch from dev to main April 6, 2023 12:57
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