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

sysroot: fix missing symlinks for blas and lapack [IO-60] #25

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

jungleraptor
Copy link
Contributor

@jungleraptor jungleraptor commented Jan 19, 2024

To support cross-compiling from x86_64 to aarch64 debian targets - we're using some scripts from the chromium
project for building sysroots directly from debian (.deb) packages.

A caveat of using these scripts is they rely on very low level plumbing commands (i.e. dpkg-deb) to handle unpacking
and installing the package contents (libs and headers), into "jail" (the sysroot).

These commands don't handle many details of the full install process. In the case of blas and lapack - they don't handle the post install step of creating symlinks at /usr/lib/${TRIPLE}/libblas.so. This is important because this location is on the linkers default search path. The reason these symlinks are not part of the package by default is because blas and lapack are virtual packages, there are several different "concrete" packages that can satisfy the dependency (i.e. libblas-dev, libopenblas-dev, etc..). It's up the the debian alternatives system (& system administrators) to handle the detail of which concrete library the libblas and liblapack symlinks actually point to.

I explored a couple of routes to try to do this in a less janky way then manually creating the links, but this ended up being a rabbit hole. Ideally we revisit the way we're building these sysroots in a more robust way.

In the meantime we can now remove the hardcoded linker paths in libraries that depend on blas and lapack:
https://github.com/swift-nav/rules_swiftnav/blob/d10814e9b8f3d3b948e3ea9c1aed9c80c2fd74cc/third_party/suitesparse.BUILD#L25. Trying to handle all the different conditions (os, arch, sysroot or no sysroot, etc..) was becoming untenable, and just didn't work at all in some cases.

Testing

I've tested the resulting sysroot build locally to verify that this mechanism of creating the links works.

@jungleraptor jungleraptor changed the title Isaac/fix sysroots sysroot: fix missing symlinks for blas and lapack Jan 19, 2024
@jungleraptor jungleraptor changed the title sysroot: fix missing symlinks for blas and lapack sysroot: fix missing symlinks for blas and lapack [IO-60] Jan 19, 2024
Copy link

@pcrumley pcrumley left a comment

Choose a reason for hiding this comment

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

This seems fine, but i really think we should remove the hardcoded paths to the exec residing at /usr/lib/{triple}/ that may be in other places. I'd hate to have the two blas/lapack libs diverge

@jungleraptor
Copy link
Contributor Author

This seems fine, but i really think we should remove the hardcoded paths to the exec residing at /usr/lib/{triple}/ that may be in other places. I'd hate to have the two blas/lapack libs diverge

Not sure what you mean but for context here's the build file we can now remove the hardcoded paths from with this change: https://github.com/swift-nav/rules_swiftnav/pull/128/files

This has to go in first though.

@jungleraptor jungleraptor merged commit 9576a06 into master Jan 19, 2024
2 checks passed
@jungleraptor jungleraptor deleted the isaac/fix-sysroots branch January 19, 2024 21:49
@pcrumley
Copy link

I thought there might be some places which points to the location from which you are copying this from in this pr and re-linking. If we don't point everything at the new place they could diverge in the future. Maybe this isn't relevant/i misunderstand

@jungleraptor
Copy link
Contributor Author

I thought there might be some places which points to the location from which you are copying this from in this pr and re-linking. If we don't point everything at the new place they could diverge in the future. Maybe this isn't relevant/i misunderstand

We were pointing to this specific location where I'm copying from here: https://github.com/swift-nav/rules_swiftnav/blob/d10814e9b8f3d3b948e3ea9c1aed9c80c2fd74cc/third_party/suitesparse.BUILD#L25, but that's the only location where we were doing that.

The reason for creating the symlinks is that the new location is on the linkers default search paths so we no longer need to include the -L args that bleed arch/os/platform details. The linker will now find the library automatically with -lblas or -llapack which is the expected for system libraries (and doesn't require us to have a bunch of selects dealing with all the permutations of where the libraries might be depending on the build variant).

@pcrumley
Copy link

Nice that is what I thought!

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.

None yet

2 participants