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.

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

Add build file for suitesparse's CHOLMOD / SPQR [OC-310] #109

Merged
merged 4 commits into from
Oct 5, 2023
Merged

Add build file for suitesparse's CHOLMOD / SPQR [OC-310] #109

merged 4 commits into from
Oct 5, 2023

Conversation

peddie
Copy link
Contributor

@peddie peddie commented Sep 19, 2023

No description provided.

@peddie peddie requested a review from jungleraptor September 19, 2023 04:11
jungleraptor
jungleraptor reviewed Sep 19, 2023
View reviewed changes
Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

You should be able to run bazel run //:buildifier to format the code.

third_party/suitesparse.BUILD Outdated Show resolved Hide resolved
third_party/suitesparse.BUILD Show resolved Hide resolved
third_party/suitesparse.BUILD Outdated Show resolved Hide resolved
third_party/suitesparse.BUILD Outdated Show resolved Hide resolved
third_party/suitesparse.BUILD Outdated Show resolved Hide resolved
third_party/suitesparse.BUILD Outdated Show resolved Hide resolved
@peddie peddie requested a review from jungleraptor September 21, 2023 22:14
@silverjam
Copy link
Contributor

cc @isaactorz

@jungleraptor
Copy link
Contributor

cc @isaactorz

There was some discussion here that moved to slack but the gist of it is we'll need to update our sysroot to include the new dependencies on blas and lapack.

Matt's switched to a different task temporarily though.

@silverjam
Copy link
Contributor

cc @isaactorz

There was some discussion here that moved to slack but the gist of it is we'll need to update our sysroot to include the new dependencies on blas and lapack.

Matt's switched to a different task temporarily though.

Thanks for the update.

@silverjam
Copy link
Contributor

I'm going to remove the review request for now

@silverjam silverjam removed the request for review from jungleraptor September 26, 2023 23:35
@peddie peddie mentioned this pull request Sep 28, 2023
Merged
@peddie peddie changed the title Add build file for suitesparse's CHOLMOD / SPQR Add build file for suitesparse's CHOLMOD / SPQR [OC-310] Sep 28, 2023
peddie added a commit to swift-nav/swift-toolchains that referenced this pull request Sep 29, 2023
@peddie
Add BLAS and LAPACK packages to sysroot [OC-310] (#21)
9a567ec 
This is required for the sparse householder QR changes in

swift-nav/orion-engine#7126
swift-nav/rules_swiftnav#109
swift-nav/albatross#453
swift-nav/cmake#167
jungleraptor
jungleraptor reviewed Sep 29, 2023
View reviewed changes
third_party/suitesparse.BUILD Outdated
@@ -300,6 +312,8 @@ cc_library(
"CHOLMOD/SuiteSparse_metis/include/",
],
linkopts = [
"-Lexternal/aarch64-sysroot/usr/lib/aarch64-linux-gnu/lapack",
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about this a little more and we're probably going to need to guard these in a select statement so that they're only put on the link line when we're actually using the sysroot.

The way bazel's build tree is structured - the external/aarch64-sysroot/ folder may be on the path even when not doing a build against a sysroot. This could cause the build to attempt to link libs for the wrong platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added the appropriate guards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

jungleraptor
jungleraptor approved these changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

LGTM assuming all testing goes well!

@peddie peddie force-pushed the peddie/suitesparse branch from 5e6ab60 to b6e2417 Compare October 3, 2023 00:09
@peddie
Copy link
Contributor Author

peddie commented Oct 3, 2023

I've updated to your version of this PR (with b6e2417 at the top) but now get errors when testing the x86_64 sysroot in orion-engine:

ERROR: /home/peddie/orion-engine/BUILD.bazel:1159:10: Linking latlon2areaid failed: (Exit 1): clang failed: error executing command (from target //:latlon2areaid) external/rules_swiftnav/cc/toolchains/llvm/x86_64-linux/wrappers/clang @bazel-out/k8-fastbuild/bin/latlon2areaid-2.params

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
ld.lld: error: unable to find library -lblas
ld.lld: error: unable to find library -llapack

@peddie
Copy link
Contributor Author

peddie commented Oct 3, 2023

OK, false alarm -- your branch did not update the sysroot pointer. I think it should be OK now.

@peddie peddie merged commit 9924b81 into main Oct 5, 2023
@peddie peddie deleted the peddie/suitesparse branch October 5, 2023 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jungleraptor jungleraptor jungleraptor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@peddie @silverjam @jungleraptor