Skip to content

Conversation

AustinSchuh
Copy link
Contributor

@AustinSchuh AustinSchuh commented Aug 9, 2025

This sets us up to use AOS, which wants @eigen to resolve, without introducing a second version or copy of eigen.

This sets us up to use AOS, which wants @eigen to resolve, without
introducing a second version or copy of eigen.
@AustinSchuh AustinSchuh requested a review from a team as a code owner August 9, 2025 19:52
@github-actions github-actions bot added component: wpimath Math library 2027 2027 target labels Aug 9, 2025
Copy link
Contributor

@pjreiniger pjreiniger left a comment

Choose a reason for hiding this comment

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

Should those licenses be pulled into the gradle and bazel publishing?

Are some of those unnecessary given the subset of eigen that is imported, or should we have had them the whole time?

@AustinSchuh
Copy link
Contributor Author

AustinSchuh commented Aug 9, 2025

Should those licenses be pulled into the gradle and bazel publishing?

Are some of those unnecessary given the subset of eigen that is imported, or should we have had them the whole time?

Those are good questions. I'll defer to Tyler on what the preferred answer is here. I suspect we should minimize changes from gradle short term.

My main goal is to make the BUILD file and targets exposed match bcr as close as possible to make it interoperate.

@calcmogul
Copy link
Member

We currently copy all the thirdparty licenses into https://github.com/wpilibsuite/allwpilib/blob/main/ThirdPartyNotices.txt, but I like the idea of upstream_utils copying the licenses into their respective thirdparty folders better though, because it's more automatable.

The library header Gradle artifacts we publish should have included the relevant license files as well. The artifact .zips are usually folder-wide, so upstream_utils copying in the license file should be sufficient.

We don't require all the Eigen licenses because we only use a subset of the source, but including them all doesn't cost much space and means we don't have to worry about it again.

@PeterJohnson
Copy link
Member

It feels weird to put them in the include/ folder instead of at the thirdparty top level? Is the reason to do this because only the include/ folder gets installed?

@AustinSchuh
Copy link
Contributor Author

It feels weird to put them in the include/ folder instead of at the thirdparty top level? Is the reason to do this because only the include/ folder gets installed?

That is because the whole repo gets put in the include folder since it is header only, and the README's are expected to be in a specific spot relative to the headers by the upstream BUILD files. It was easier to embrace it than be unique.

@auscompgeek
Copy link
Member

Yep, the eigen BCR module just takes the git repo and overlays the BUILD file at its repo root.

Subject: [PATCH 3/3] Add build files from bzlmod for eigen

This makes it so our vendored version of eigen matches the same API as
the upstream eigen in bcr.
Copy link
Member

Choose a reason for hiding this comment

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

non-blocking: Might be worth leaving a link to the BCR upstream here?

@PeterJohnson PeterJohnson merged commit 5c719ce into wpilibsuite:2027 Oct 3, 2025
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2027 2027 target component: wpimath Math library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants