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

box2d: Do not force warnings as errors by default #25815

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

skhaz
Copy link
Contributor

@skhaz skhaz commented Nov 3, 2024

Summary

Changes to recipe: box2d/3.0.0

Motivation

This (hopefully) closes #25797

Details

Box2D compiles on Linux, Windows, and macOS, but not for WebAssembly using emsdk. This PR attempts to address the compilation error I’m encountering and should not impact other users.


@CLAassistant
Copy link

CLAassistant commented Nov 3, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ❌

Warning

Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement.

Sorry, the build is only launched for Access Request users. You can request access writing in this issue.

Conan v2 pipeline ❌

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping @conan-io/barbarians on the PR and we will help you.

Sorry, the build is only launched for Access Request users. You can request access writing in this issue.

@skhaz
Copy link
Contributor Author

skhaz commented Nov 4, 2024

Please help @memsharded

jcar87
jcar87 previously requested changes Nov 4, 2024
Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

see comment

@@ -78,6 +78,7 @@ def source(self):
def generate(self):
tc = CMakeToolchain(self)
tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0077"] = "NEW"
tc.variables["CMAKE_CXX_FLAGS"] = "-O3 -ftree-vectorize -fopt-info-vec -Wno-pass-failed"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @skhaz - thanks for your contribution.

Doing it this way does not take into account the compiler - they are passed unconditionally to all compilers regardless of whether they understand those particular flags. Additionally, -O3 would trample and override the user's build_type setting - since it is being applied unconditionally.

This looks like a bug in the upstream recipe itself - I would first ensure that it is, at the very least, reported upstream to the maintainers: https://github.com/erincatto/box2d

In this instance, however I would possibly advise simply setting

tc.cache_variables["CMAKE_COMPILE_WARNING_AS_ERROR"] = False

since this is failing on emscriptem because this setting is too strict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I have updated my pull request and raised an issue on box2d repository.

@jcar87 jcar87 self-assigned this Nov 4, 2024
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks @skhaz happy to see that you reported an issue upstream, I'm sure they'll appreciate it :)

The current workaround looks good to me to get this to compile, and if a new version is generated with upstream's fix, we'll be happy to add it :)

@skhaz
Copy link
Contributor Author

skhaz commented Nov 4, 2024

Thank you @AbrilRBS. I will keep an eye and when they provide a fix I will let you know.

@jcar87 jcar87 dismissed their stale review November 4, 2024 16:33

addressed

@jcar87
Copy link
Contributor

jcar87 commented Nov 4, 2024

Thanks @skhaz ! Running this on CI - please let us know if with that setting, it at least lets you proceed :D

@skhaz
Copy link
Contributor Author

skhaz commented Nov 4, 2024

Is there a way to test this branch locally, I mean, instead of conan pulling the packages specs from the main repository/internet, fetch from my branch?

@jcar87
Copy link
Contributor

jcar87 commented Nov 4, 2024

Is there a way to test this branch locally, I mean, instead of conan pulling the packages specs from the main repository/internet, fetch from my branch?

cd conan-center-index/recipes/box2d/all
conan export . --version=2.4.2

and then add --build=missing when you are solving the graph for a consumer (e.g. conan install)
or you can call conan create . --version=2.4.2 from the same directory and that will trigger the build

@jcar87 jcar87 changed the title Add flag to vectorize and bypass warning for Box2D box2d: Do not force warnings as errors by default Nov 4, 2024
@jcar87 jcar87 merged commit bfb4854 into conan-io:master Nov 4, 2024
9 checks passed
@skhaz skhaz deleted the box2d-vectorize branch November 4, 2024 17:04
@lindequist
Copy link

First of all, sorry if I commenting in the wrong place. I was also trying to use box2d/3.0.0 with emsdk/3.1.50, and I also got errors a la:

.conan2/p/b/box2d6487a51d42777/b/src/src/contact_solver.c:810:6: error: loop not vectorized: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering [-Werror,-Wpass-failed=transform-warning]
  810 | void b2WarmStartContactsTask( int startIndex, int endIndex, b2StepContext* context, int colorIndex )

I tried with the changes merged into master here, but it didn't fix the issue for me. Then, just for testing, I checked out conan-center-index and added this line instead in recipes/box2d/all/conanfile.py:

tc.cache_variables["CMAKE_C_FLAGS"] = "-Wno-error=pass-failed"

After exporting using conan export . --version=3.0.0, I was able to build the box2d dependency, and the warning that was treated as an error before is now just a warning.

@skhaz
Copy link
Contributor Author

skhaz commented Nov 10, 2024

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.

[package] box2d/3.0.0: error: loop not vectorized
6 participants