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

Update github actions #46

Merged
merged 20 commits into from
Oct 23, 2024
Merged

Update github actions #46

merged 20 commits into from
Oct 23, 2024

Conversation

qnzhou
Copy link
Contributor

@qnzhou qnzhou commented Oct 23, 2024

Summary:

  • Update cibuildwheel github action to use manylinux_2_28 image.
  • Handle warning related to Poisson module.
  • Bypass GCC12 bug.

cmake/recipes/external/poissonrecon.cmake Outdated Show resolved Hide resolved
# Bypass the following GCC bug.
# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106247
target_compile_options(Eigen3_Eigen INTERFACE
"-Wno-array-bounds"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here I'm tempted to use the warnoff/warnon mechanism rather than having this flag spill into our own codebase. But given how widely we use Eigen that sounds really annoying...

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 am a bit worried about this due to exactly this reason, but it is worth a try.

Copy link
Contributor

Choose a reason for hiding this comment

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

One option would be to use a "proxy" header to fence the include with warnoff/warnon. We can have one proxy header for Dense/Sparse/Geometry.

Do you know if the warning is triggered when the template is instantiated? (in our own codebase), or when the header is parsed (i.e. inside the Eigen header)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is triggered in our own code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok it seems that this warning is unreliable with gcc. Let's add it as a PRIVATE warning in the main Lagrange targets then (e.g. when we define our target via lagrange_add_module()), to avoid spilling out into client code.

Copy link
Contributor Author

@qnzhou qnzhou Oct 23, 2024

Choose a reason for hiding this comment

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

Can you take a look at 0aca2cb? It seems there are only a few places that triggers this warning, and we can put warning ignore guards around those places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! That's even better.

@qnzhou qnzhou enabled auto-merge (squash) October 23, 2024 22:34
@qnzhou qnzhou merged commit ffec583 into main Oct 23, 2024
22 checks passed
@qnzhou qnzhou deleted the qnzhou/wheel branch October 23, 2024 22:44
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